Dont include imports for time:hour and time:minute dimensions (#4504)

* Dont include imports for time:hour and time:minute dimensions

Also include more information about import warnings in query results

* Update lib/plausible/stats/query_result.ex

Co-authored-by: RobertJoonas <56999674+RobertJoonas@users.noreply.github.com>

* Revert patch

* Imported disabled graph notice (#4522)

* add explicit skip_imported_reason for unsupported interval

* stop returning information about imports from main_graph

* return warning about interval in Stats API Timeseries

* display warning bubble about interval too short for imported data

* update changelog

* improve styling of the exclamation circle icon

* return tuple from timeseries instead of map

* rename variable

* Update CHANGELOG.md

---------

Co-authored-by: RobertJoonas <56999674+RobertJoonas@users.noreply.github.com>

CHANGELOG.md

@@ -43,6 +43,7 @@ All notable changes to this project will be documented in this file.
 - Fix access to Stats API feature in CE plausible/analytics#4244
 - Fix filter suggestions when same filter previously applied
 - Fix MX lookup when using relays with Bamboo.Mua plausible/analytics#4350
+- Don't include imports when showing time series hourly interval. Previously imported data was shown each midnight
 - Fix property filter suggestions 500 error when property hasn't been selected
 - Bamboo.Mua: add Date and Message-ID headers if missing plausible/analytics#4474
 

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

@@ -14,6 +14,7 @@ import { isComparisonEnabled } from '../../query-time-periods';
 import LineGraphWithRouter from './line-graph';
 import { useQueryContext } from '../../query-context';
 import { useSiteContext } from '../../site-context';
+import { ExclamationCircleIcon } from '@heroicons/react/24/outline'
 
 function fetchTopStats(site, query) {
   const q = { ...query }
@@ -135,6 +136,22 @@ export default function VisitorGraph({ updateImportedDataInView }) {
     }
   }
 
+  function importedSwitchVisible() {
+    return !!topStatData?.with_imported_switch && topStatData?.with_imported_switch.visible
+  }
+
+  function renderImportedIntervalUnsupportedWarning() {
+    const unsupportedInterval = ['hour', 'minute'].includes(getCurrentInterval(site, query))
+    const showingImported = importedSwitchVisible() && query.with_imported === true
+
+    return (
+      <FadeIn show={showingImported && unsupportedInterval} className="h-6 mr-1">
+        <span tooltip={"Inteval is too short to graph imported data"}>
+          <ExclamationCircleIcon className="w-6 h-6 text-gray-700 dark:text-gray-300" />
+        </span>
+      </FadeIn>
+    )
+  }
 
   return (
     <div className={"relative w-full mt-2 bg-white rounded shadow-xl dark:bg-gray-825"}>
@@ -150,9 +167,10 @@ export default function VisitorGraph({ updateImportedDataInView }) {
         <div className="relative px-2">
           {graphRefreshing && renderLoader()}
           <div className="absolute right-4 -top-8 py-1 flex items-center">
+            {renderImportedIntervalUnsupportedWarning()}
             {!isRealtime && <StatsExport />}
             <SamplingNotice samplePercent={topStatData} />
-            {!!topStatData?.with_imported_switch && topStatData?.with_imported_switch.visible &&
+            {importedSwitchVisible() &&
               <WithImportedSwitch
                 tooltipMessage={topStatData.with_imported_switch.tooltip_msg}
                 disabled={!topStatData.with_imported_switch.togglable}

lib/plausible/stats/query.ex

@@ -141,12 +141,26 @@ defmodule Plausible.Stats.Query do
           :ok | {:error, :no_imported_data | :out_of_range | :unsupported_query | :not_requested}
   def ensure_include_imported(query, requested?) do
     cond do
-      is_nil(query.latest_import_end_date) -> {:error, :no_imported_data}
-      query.period in ["realtime", "30m"] -> {:error, :unsupported_query}
-      Date.after?(query.date_range.first, query.latest_import_end_date) -> {:error, :out_of_range}
-      not Imported.schema_supports_query?(query) -> {:error, :unsupported_query}
-      not requested? -> {:error, :not_requested}
-      true -> :ok
+      is_nil(query.latest_import_end_date) ->
+        {:error, :no_imported_data}
+
+      query.period in ["realtime", "30m"] ->
+        {:error, :unsupported_query}
+
+      "time:minute" in query.dimensions or "time:hour" in query.dimensions ->
+        {:error, :unsupported_interval}
+
+      Date.after?(query.date_range.first, query.latest_import_end_date) ->
+        {:error, :out_of_range}
+
+      not Imported.schema_supports_query?(query) ->
+        {:error, :unsupported_query}
+
+      not requested? ->
+        {:error, :not_requested}
+
+      true ->
+        :ok
     end
   end
 

lib/plausible/stats/query_result.ex

@@ -68,11 +68,17 @@ defmodule Plausible.Stats.QueryResult do
   @imports_unsupported_query_warning "Imported stats are not included in the results because query parameters are not supported. " <>
                                        "For more information, see: https://plausible.io/docs/stats-api#filtering-imported-stats"
 
+  @imports_unsupported_interval_warning "Imported stats are not included because the time dimension (i.e. the interval) is too short."
+
   defp meta(query) do
     %{
-      warning:
+      imports_included: if(query.include.imports, do: query.include_imported, else: nil),
+      imports_skip_reason:
+        if(query.skip_imported_reason, do: Atom.to_string(query.skip_imported_reason), else: nil),
+      imports_warning:
         case query.skip_imported_reason do
           :unsupported_query -> @imports_unsupported_query_warning
+          :unsupported_interval -> @imports_unsupported_interval_warning
           _ -> nil
         end,
       time_labels:

lib/plausible/stats/timeseries.ex

@@ -38,11 +38,17 @@ defmodule Plausible.Stats.Timeseries do
 
     q = SQL.QueryBuilder.build(query_with_metrics, site)
 
-    q
-    |> ClickhouseRepo.all(query: query)
-    |> QueryResult.from(site, query_with_metrics)
-    |> build_timeseries_result(query_with_metrics, currency)
-    |> transform_keys(%{group_conversion_rate: :conversion_rate})
+    query_result =
+      q
+      |> ClickhouseRepo.all(query: query)
+      |> QueryResult.from(site, query_with_metrics)
+
+    timeseries_result =
+      query_result
+      |> build_timeseries_result(query_with_metrics, currency)
+      |> transform_keys(%{group_conversion_rate: :conversion_rate})
+
+    {timeseries_result, query_result.meta}
   end
 
   defp time_dimension(query), do: Map.fetch!(@time_dimension, query.interval)

lib/plausible_web/controllers/api/external_stats_controller.ex

@@ -273,8 +273,13 @@ defmodule PlausibleWeb.Api.ExternalStatsController do
          :ok <- validate_filters(site, query.filters),
          {:ok, metrics} <- parse_and_validate_metrics(params, query),
          :ok <- ensure_custom_props_access(site, query) do
-      graph = Plausible.Stats.timeseries(site, query, metrics)
-      payload = maybe_add_warning(%{results: graph}, query)
+      {results, meta} = Plausible.Stats.timeseries(site, query, metrics)
+
+      payload =
+        case meta[:imports_warning] do
+          nil -> %{results: results}
+          warning -> %{results: results, warning: warning}
+        end
 
       json(conn, payload)
     else

lib/plausible_web/controllers/api/stats_controller.ex

@@ -107,17 +107,17 @@ defmodule PlausibleWeb.Api.StatsController do
          params <- realtime_period_to_30m(params),
          query = Query.from(site, params, debug_metadata(conn)),
          {:ok, metric} <- parse_and_validate_graph_metric(params, query) do
-      timeseries_result = Stats.timeseries(site, query, [metric])
+      {timeseries_result, _meta} = Stats.timeseries(site, query, [metric])
 
       comparison_opts = parse_comparison_opts(params)
 
-      {comparison_query, comparison_result} =
+      comparison_result =
         case Comparisons.compare(site, query, params["comparison"], comparison_opts) do
           {:ok, comparison_query} ->
-            {comparison_query, Stats.timeseries(site, comparison_query, [metric])}
+            Stats.timeseries(site, comparison_query, [metric]) |> elem(0)
 
           {:error, :not_supported} ->
-            {nil, nil}
+            nil
         end
 
       labels = label_timeseries(timeseries_result, comparison_result)
@@ -131,8 +131,6 @@ defmodule PlausibleWeb.Api.StatsController do
         comparison_plot: comparison_result && plot_timeseries(comparison_result, metric),
         comparison_labels: comparison_result && label_timeseries(comparison_result, nil),
         present_index: present_index,
-        includes_imported: includes_imported?(query, comparison_query),
-        imports_exist: site.complete_import_ids != [],
         full_intervals: full_intervals
       })
     else

lib/plausible_web/controllers/stats_controller.ex

@@ -173,6 +173,7 @@ defmodule PlausibleWeb.StatsController do
     prepend_column_headers = fn data -> [column_headers | data] end
 
     Plausible.Stats.timeseries(site, query, metrics)
+    |> elem(0)
     |> Enum.map(map_bucket_to_row)
     |> prepend_column_headers.()
     |> NimbleCSV.RFC4180.dump_to_iodata()

test/plausible/stats/query_result_test.exs

@@ -41,7 +41,10 @@ defmodule Plausible.Stats.QueryResultTest do
     assert query_result_json == """
            {
              "results": [],
-             "meta": {},
+             "meta": {
+               "imports_included": false,
+               "imports_skip_reason": "no_imported_data"
+             },
              "query": {
                "site_id": "dummy.site",
                "metrics": [

test/plausible_web/controllers/api/external_stats_controller/query_imported_test.exs

@@ -31,7 +31,8 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryImportedTest do
       conn2 = post(conn, "/api/v2/query", Map.put(query_params, "include", %{"imports" => true}))
 
       assert json_response(conn2, 200)["results"] == [%{"metrics" => [2], "dimensions" => []}]
-      refute json_response(conn2, 200)["meta"]["warning"]
+      assert json_response(conn2, 200)["meta"]["imports_included"]
+      refute json_response(conn2, 200)["meta"]["imports_warning"]
     end
   end
 
@@ -425,7 +426,8 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryImportedTest do
                  %{"dimensions" => ["https://one.com"], "metrics" => [3, 6, 30]}
                ]
 
-        refute json_response(conn, 200)["meta"]["warning"]
+        assert json_response(conn, 200)["meta"]["imports_included"]
+        refute json_response(conn, 200)["meta"]["imports_warning"]
       end
     end
 
@@ -477,7 +479,8 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryImportedTest do
                  %{"dimensions" => ["/one"], "metrics" => [3, 6, 30]}
                ]
 
-        refute json_response(conn, 200)["meta"]["warning"]
+        assert json_response(conn, 200)["meta"]["imports_included"]
+        refute json_response(conn, 200)["meta"]["imports_warning"]
       end
     end
 
@@ -519,6 +522,73 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryImportedTest do
              ]
     end
 
+    test "includes imported data for time:day dimension", %{
+      conn: conn,
+      site: site
+    } do
+      site_import = insert(:site_import, site: site)
+
+      insert(:goal, event_name: "Signup", site: site)
+
+      populate_stats(site, site_import.id, [
+        build(:pageview, timestamp: ~N[2021-01-01 00:00:00]),
+        build(:pageview, timestamp: ~N[2021-01-01 00:10:00]),
+        build(:pageview, timestamp: ~N[2021-01-01 23:59:00]),
+        build(:imported_visitors, date: ~D[2021-01-01], visitors: 5)
+      ])
+
+      conn =
+        post(conn, "/api/v2/query", %{
+          "site_id" => site.domain,
+          "metrics" => ["visitors"],
+          "date_range" => ["2021-01-01", "2021-01-02"],
+          "dimensions" => ["time:day"],
+          "include" => %{"imports" => true}
+        })
+
+      assert json_response(conn, 200)["results"] == [
+               %{"dimensions" => ["2021-01-01"], "metrics" => [8]}
+             ]
+
+      assert json_response(conn, 200)["meta"] == %{"imports_included" => true}
+    end
+
+    test "adds a warning when time:hour dimension", %{
+      conn: conn,
+      site: site
+    } do
+      site_import = insert(:site_import, site: site)
+
+      insert(:goal, event_name: "Signup", site: site)
+
+      populate_stats(site, site_import.id, [
+        build(:pageview, timestamp: ~N[2021-01-01 00:00:00]),
+        build(:pageview, timestamp: ~N[2021-01-01 00:10:00]),
+        build(:pageview, timestamp: ~N[2021-01-01 23:59:00]),
+        build(:imported_visitors, date: ~D[2021-01-01], visitors: 5)
+      ])
+
+      conn =
+        post(conn, "/api/v2/query", %{
+          "site_id" => site.domain,
+          "metrics" => ["visitors"],
+          "date_range" => ["2021-01-01", "2021-01-02"],
+          "dimensions" => ["time:hour"],
+          "include" => %{"imports" => true}
+        })
+
+      assert json_response(conn, 200)["results"] == [
+               %{"dimensions" => ["2021-01-01 00:00:00"], "metrics" => [2]},
+               %{"dimensions" => ["2021-01-01 23:00:00"], "metrics" => [1]}
+             ]
+
+      refute json_response(conn, 200)["meta"]["imports_included"]
+      assert json_response(conn, 200)["meta"]["imports_skip_reason"] == "unsupported_interval"
+
+      assert json_response(conn, 200)["meta"]["imports_warning"] =~
+               "Imported stats are not included because the time dimension (i.e. the interval) is too short."
+    end
+
     test "adds a warning when query params are not supported for imported data", %{
       conn: conn,
       site: site
@@ -552,7 +622,10 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryImportedTest do
                %{"dimensions" => ["large"], "metrics" => [1]}
              ]
 
-      assert json_response(conn, 200)["meta"]["warning"] =~
+      refute json_response(conn, 200)["meta"]["imports_included"]
+      assert json_response(conn, 200)["meta"]["imports_skip_reason"] == "unsupported_query"
+
+      assert json_response(conn, 200)["meta"]["imports_warning"] =~
                "Imported stats are not included in the results because query parameters are not supported."
     end
 
@@ -579,7 +652,10 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryImportedTest do
           "include" => %{"imports" => true}
         })
 
-      refute json_response(conn, 200)["meta"]["warning"]
+      assert json_response(conn, 200)["meta"] == %{
+               "imports_included" => false,
+               "imports_skip_reason" => "no_imported_data"
+             }
     end
 
     test "does not add a warning when import is out of queried date range", %{
@@ -611,7 +687,10 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryImportedTest do
           "include" => %{"imports" => true}
         })
 
-      refute json_response(conn, 200)["meta"]["warning"]
+      assert json_response(conn, 200)["meta"] == %{
+               "imports_included" => false,
+               "imports_skip_reason" => "out_of_range"
+             }
     end
 
     test "applies multiple filters if the properties belong to the same table", %{
@@ -677,7 +756,8 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryImportedTest do
                "meta" => meta
              } = json_response(conn, 200)
 
-      assert meta["warning"] =~ "Imported stats are not included in the results"
+      refute json_response(conn, 200)["meta"]["imports_included"]
+      assert meta["imports_warning"] =~ "Imported stats are not included in the results"
     end
 
     test "imported country, region and city data",

test/plausible_web/controllers/api/external_stats_controller/timeseries_test.exs

@@ -1611,6 +1611,34 @@ defmodule PlausibleWeb.Api.ExternalStatsController.TimeseriesTest do
                "Imported stats are not included in the results because query parameters are not supported."
     end
 
+    test "is not included for a day period and an appropriate warning is returned", %{
+      conn: conn,
+      site: site
+    } do
+      site_import = insert(:site_import, site: site)
+
+      populate_stats(site, site_import.id, [
+        build(:imported_visitors, visitors: 1, date: ~D[2021-01-01])
+      ])
+
+      conn =
+        conn
+        |> get("/api/v1/stats/timeseries", %{
+          "site_id" => site.domain,
+          "period" => "day",
+          "metrics" => "visitors",
+          "date" => "2021-01-01",
+          "with_imported" => "true"
+        })
+
+      assert %{"results" => results, "warning" => warning} = json_response(conn, 200)
+
+      Enum.each(results, &assert(&1["visitors"] == 0))
+
+      assert warning ==
+               "Imported stats are not included because the time dimension (i.e. the interval) is too short."
+    end
+
     test "does not add a warning when there are no site imports", %{conn: conn, site: site} do
       populate_stats(site, [
         build(:pageview,

test/plausible_web/controllers/api/stats_controller/imported_test.exs

@@ -80,8 +80,7 @@ defmodule PlausibleWeb.Api.StatsController.ImportedTest do
             "/api/stats/#{site.domain}/main-graph?period=month&date=2021-01-01&with_imported=true"
           )
 
-        assert %{"plot" => plot, "imports_exist" => true} =
-                 json_response(conn, 200)
+        assert %{"plot" => plot} = json_response(conn, 200)
 
         assert Enum.count(plot) == 31
         assert List.first(plot) == 2
@@ -129,8 +128,7 @@ defmodule PlausibleWeb.Api.StatsController.ImportedTest do
             "/api/stats/#{site.domain}/main-graph?period=month&date=2021-01-01&with_imported=true&interval=week"
           )
 
-        assert %{"plot" => plot, "imports_exist" => true} =
-                 json_response(conn, 200)
+        assert %{"plot" => plot} = json_response(conn, 200)
 
         assert Enum.count(plot) == 5
         assert List.first(plot) == 2

test/plausible_web/controllers/api/stats_controller/main_graph_test.exs

@@ -74,10 +74,13 @@ defmodule PlausibleWeb.Api.StatsController.MainGraphTest do
         )
 
       zeroes = List.duplicate(0, 30)
-      assert %{"plot" => ^zeroes, "includes_imported" => false} = json_response(conn, 200)
+      assert %{"plot" => ^zeroes} = json_response(conn, 200)
     end
 
-    test "displays visitors for a day with imported data", %{conn: conn, site: site} do
+    test "imported data is not included for hourly interval", %{
+      conn: conn,
+      site: site
+    } do
       populate_stats(site, [
         build(:pageview, timestamp: ~N[2021-01-01 00:00:00]),
         build(:pageview, timestamp: ~N[2021-01-31 00:00:00]),
@@ -91,10 +94,9 @@ defmodule PlausibleWeb.Api.StatsController.MainGraphTest do
           "/api/stats/#{site.domain}/main-graph?period=day&date=2021-01-01&with_imported=true"
         )
 
-      assert %{"plot" => plot, "imports_exist" => true, "includes_imported" => true} =
-               json_response(conn, 200)
+      assert %{"plot" => plot} = json_response(conn, 200)
 
-      assert plot == [2] ++ List.duplicate(0, 23)
+      assert plot == [1] ++ List.duplicate(0, 23)
     end
 
     test "displays hourly stats in configured timezone", %{conn: conn, user: user} do
@@ -158,8 +160,7 @@ defmodule PlausibleWeb.Api.StatsController.MainGraphTest do
           "/api/stats/#{site.domain}/main-graph?period=month&date=2021-01-01&with_imported=true"
         )
 
-      assert %{"plot" => plot, "imports_exist" => true, "includes_imported" => true} =
-               json_response(conn, 200)
+      assert %{"plot" => plot} = json_response(conn, 200)
 
       assert Enum.count(plot) == 31
       assert List.first(plot) == 2
@@ -179,8 +180,7 @@ defmodule PlausibleWeb.Api.StatsController.MainGraphTest do
           "/api/stats/#{site.domain}/main-graph?period=month&date=2021-01-01&with_imported=true"
         )
 
-      assert %{"plot" => plot, "imports_exist" => true, "includes_imported" => true} =
-               json_response(conn, 200)
+      assert %{"plot" => plot} = json_response(conn, 200)
 
       assert Enum.count(plot) == 31
       assert List.first(plot) == 1
@@ -1216,12 +1216,7 @@ defmodule PlausibleWeb.Api.StatsController.MainGraphTest do
           "/api/stats/#{site.domain}/main-graph?period=year&date=2021-01-01&with_imported=true&comparison=year_over_year&interval=month"
         )
 
-      assert %{
-               "plot" => plot,
-               "comparison_plot" => comparison_plot,
-               "imports_exist" => true,
-               "includes_imported" => true
-             } = json_response(conn, 200)
+      assert %{"plot" => plot, "comparison_plot" => comparison_plot} = json_response(conn, 200)
 
       assert 4 == Enum.sum(plot)
       assert 2 == Enum.sum(comparison_plot)
@@ -1262,12 +1257,7 @@ defmodule PlausibleWeb.Api.StatsController.MainGraphTest do
           "/api/stats/#{site.domain}/main-graph?period=year&date=2021-01-01&with_imported=false&comparison=year_over_year&interval=month"
         )
 
-      assert %{
-               "plot" => plot,
-               "comparison_plot" => comparison_plot,
-               "imports_exist" => true,
-               "includes_imported" => false
-             } = json_response(conn, 200)
+      assert %{"plot" => plot, "comparison_plot" => comparison_plot} = json_response(conn, 200)
 
       assert 4 == Enum.sum(plot)
       assert 0 == Enum.sum(comparison_plot)
@@ -1292,12 +1282,8 @@ defmodule PlausibleWeb.Api.StatsController.MainGraphTest do
           "/api/stats/#{site.domain}/main-graph?period=7d&date=2021-01-14&comparison=previous_period&metric=conversion_rate&filters=#{filters}"
         )
 
-      assert %{
-               "plot" => this_week_plot,
-               "comparison_plot" => last_week_plot,
-               "imports_exist" => true,
-               "includes_imported" => false
-             } = json_response(conn, 200)
+      assert %{"plot" => this_week_plot, "comparison_plot" => last_week_plot} =
+               json_response(conn, 200)
 
       assert this_week_plot == [50.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0]
       assert last_week_plot == [33.3, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0]