Two fixes related to main graph and revenue metrics

lib/plausible/stats/imported/imported.ex

@@ -1,4 +1,9 @@
 defmodule Plausible.Stats.Imported do
+  @moduledoc """
+  Module defining functions that merge imported query into a native one,
+  and also functions that decide whether imported data is eligible for
+  a given query.
+  """
   use Plausible.ClickhouseRepo
   use Plausible.Stats.SQL.Fragments
 
@@ -360,7 +365,9 @@ defmodule Plausible.Stats.Imported do
     :bounce_rate,
     :conversion_rate,
     :group_conversion_rate,
-    :time_on_page
+    :time_on_page,
+    :total_revenue,
+    :average_revenue
   ]
 
   defp can_order_by?(query) do

lib/plausible/stats/parsed_query_params.ex

@@ -1,6 +1,8 @@
 defmodule Plausible.Stats.ParsedQueryParams do
   @moduledoc false
 
+  alias Plausible.Stats.Query
+
   defstruct input_date_range: nil,
             # `relative_date` is a convenience currently exclusive to the internal
             # dashboard API for constructing datetime ranges. It adds the ability
@@ -20,6 +22,11 @@ defmodule Plausible.Stats.ParsedQueryParams do
             # the behaviour of legacy endpoints like top_stats.
             skip_goal_existence_check: false
 
+  def to_query!(%__MODULE__{} = parsed_query_params) do
+    query_fields = %Query{} |> Map.from_struct() |> Map.keys()
+    struct!(%Query{}, Map.take(parsed_query_params, query_fields))
+  end
+
   def new!(params) when is_map(params) do
     struct!(__MODULE__, Map.to_list(params))
   end

lib/plausible/stats/query_builder.ex

@@ -30,7 +30,7 @@ defmodule Plausible.Stats.QueryBuilder do
 
   def build(site, %ParsedQueryParams{} = parsed_query_params, debug_metadata) do
     with {:ok, parsed_query_params} <- resolve_segments_in_filters(parsed_query_params, site),
-         query = do_build(parsed_query_params, site, debug_metadata),
+         {:ok, query} <- do_build(parsed_query_params, site, debug_metadata),
          :ok <- validate_order_by(query),
          :ok <- validate_custom_props_access(site, query),
          :ok <- validate_case_sensitive_filter_modifier(query),
@@ -150,49 +150,60 @@ defmodule Plausible.Stats.QueryBuilder do
   end
 
   defp do_build(parsed_query_params, site, debug_metadata) do
-    now = parsed_query_params.now || DateTime.utc_now(:second)
+    parsed_query_params
+    |> ParsedQueryParams.to_query!()
+    |> set_now()
+    |> set_utc_time_range(site, Map.get(parsed_query_params, :relative_date))
+    |> set_preloaded_goals_and_revenue(site)
+    |> Query.set(
+      site_id: site.id,
+      site_native_stats_start_at: site.native_stats_start_at,
+      timezone: site.timezone,
+      consolidated_site_ids: get_consolidated_site_ids(site),
+      debug_metadata: debug_metadata
+    )
+    |> maybe_drop_revenue_metrics()
+  end
 
-    %ParsedQueryParams{
-      input_date_range: input_date_range,
-      relative_date: relative_date,
-      metrics: metrics,
-      filters: filters,
-      dimensions: dimensions
-    } = parsed_query_params
+  defp set_now(%Query{now: nil} = query), do: Query.set(query, now: DateTime.utc_now(:second))
+  defp set_now(query), do: query
 
-    relative_date = relative_date || Times.to_date(now, site.timezone)
+  defp set_utc_time_range(query, site, relative_date) do
+    relative_date = relative_date || Times.to_date(query.now, site.timezone)
 
     utc_time_range =
-      input_date_range
-      |> build_datetime_range(site, relative_date, now)
+      query.input_date_range
+      |> build_datetime_range(site, relative_date, query.now)
       |> DateTimeRange.to_timezone("Etc/UTC")
 
-    {preloaded_goals, revenue_warning, revenue_currencies} =
-      preload_goals_and_revenue(site, metrics, filters, dimensions)
+    Query.set(query, utc_time_range: utc_time_range)
+  end
 
-    consolidated_site_ids = get_consolidated_site_ids(site)
+  defp set_preloaded_goals_and_revenue(query, site) do
+    {preloaded_goals, revenue_warning, revenue_currencies} =
+      preload_goals_and_revenue(site, query.metrics, query.filters, query.dimensions)
 
-    struct!(%Query{},
-      now: now,
-      input_date_range: input_date_range,
-      utc_time_range: utc_time_range,
-      site_id: site.id,
-      metrics: metrics,
-      dimensions: dimensions,
-      filters: filters,
-      order_by: parsed_query_params.order_by,
-      pagination: parsed_query_params.pagination,
-      include: parsed_query_params.include,
-      site_native_stats_start_at: site.native_stats_start_at,
-      consolidated_site_ids: consolidated_site_ids,
-      timezone: site.timezone,
+    Query.set(query,
       preloaded_goals: preloaded_goals,
       revenue_warning: revenue_warning,
-      revenue_currencies: revenue_currencies,
-      debug_metadata: debug_metadata
+      revenue_currencies: revenue_currencies
     )
   end
 
+  def preload_goals_and_revenue(site, metrics, filters, dimensions) do
+    preloaded_goals =
+      Plausible.Stats.Goals.preload_needed_goals(site, dimensions, filters)
+
+    {revenue_warning, revenue_currencies} =
+      preload_revenue(site, preloaded_goals, metrics, dimensions)
+
+    {
+      preloaded_goals,
+      revenue_warning,
+      revenue_currencies
+    }
+  end
+
   on_ee do
     def get_consolidated_site_ids(%Plausible.Site{} = site) do
       if Plausible.Sites.consolidated?(site) do
@@ -219,20 +230,6 @@ defmodule Plausible.Stats.QueryBuilder do
     struct!(query, comparison_utc_time_range: datetime_range)
   end
 
-  def preload_goals_and_revenue(site, metrics, filters, dimensions) do
-    preloaded_goals =
-      Plausible.Stats.Goals.preload_needed_goals(site, dimensions, filters)
-
-    {revenue_warning, revenue_currencies} =
-      preload_revenue(site, preloaded_goals, metrics, dimensions)
-
-    {
-      preloaded_goals,
-      revenue_warning,
-      revenue_currencies
-    }
-  end
-
   on_ee do
     alias Plausible.Stats.Goal.Revenue
 
@@ -241,8 +238,7 @@ defmodule Plausible.Stats.QueryBuilder do
     end
 
     defp validate_revenue_metrics_access(site, query) do
-      if Revenue.requested?(query.metrics) and not Revenue.available?(site) and
-           not query.include.drop_unavailable_revenue_metrics do
+      if Revenue.requested?(query.metrics) and not Revenue.available?(site) do
         {:error,
          %QueryError{
            code: :feature_access,
@@ -252,10 +248,32 @@ defmodule Plausible.Stats.QueryBuilder do
         :ok
       end
     end
+
+    defp maybe_drop_revenue_metrics(
+           %Query{
+             include: %QueryInclude{drop_unavailable_revenue_metrics: true},
+             revenue_currencies: revenue_currencies
+           } = query
+         )
+         when map_size(revenue_currencies) == 0 do
+      if Enum.all?(query.metrics, &(&1 in Revenue.revenue_metrics())) do
+        {:error,
+         %QueryError{
+           code: :all_metrics_dropped,
+           message: "Revenue metrics were dropped and no other metrics were left to query."
+         }}
+      else
+        {:ok, Query.set(query, metrics: query.metrics -- Revenue.revenue_metrics())}
+      end
+    end
+
+    defp maybe_drop_revenue_metrics(%Query{} = query), do: {:ok, query}
   else
     defp preload_revenue(_site, _preloaded_goals, _metrics, _dimensions), do: {nil, %{}}
 
     defp validate_revenue_metrics_access(_site, _query), do: :ok
+
+    defp maybe_drop_revenue_metrics(query), do: {:ok, query}
   end
 
   defp validate_order_by(query) do

lib/plausible/stats/query_optimizer.ex

@@ -24,7 +24,7 @@ defmodule Plausible.Stats.QueryOptimizer do
     2. Adds a missing order_by clause to a query
     3. Updating "time" dimension in order_by to the right granularity
     4. Updates event:hostname filters to also apply on visit level for sane results.
-    5. Removes revenue metrics from dashboard queries if not requested, present or unavailable for the site.
+    5. [DEPRECATED AND WILL BE REMOVED] Removes revenue metrics from legacy queries if ineligible
     6. Trims the date range to the current time if query.include.trim_relative_date_range is true.
     7. Sets the join_type for the query based on the query.
 

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

@@ -2037,6 +2037,67 @@ defmodule PlausibleWeb.Api.StatsController.MainGraphTest do
     end
   end
 
+  describe "revenue metric dropped" do
+    @describetag :ee_only
+    setup [:create_user, :log_in, :create_site]
+
+    for revenue_metric <- ["total_revenue", "average_revenue"] do
+      test "#{revenue_metric} without goal filter", %{
+        conn: conn,
+        site: site
+      } do
+        response =
+          do_query_fail(conn, site, %{
+            "date_range" => "day",
+            "metrics" => [unquote(revenue_metric)],
+            "dimensions" => ["time:hour"]
+          })
+
+        assert %{"error" => error} = json_response(response, 400)
+        assert error =~ "Revenue metrics were dropped"
+      end
+
+      test "#{revenue_metric} with filtered goals mixing currencies", %{
+        conn: conn,
+        site: site
+      } do
+        insert(:goal, site: site, event_name: "PurchaseEUR", currency: "EUR")
+        insert(:goal, site: site, event_name: "PurchaseUSD", currency: "USD")
+
+        response =
+          do_query_fail(conn, site, %{
+            "date_range" => "day",
+            "metrics" => [unquote(revenue_metric)],
+            "filters" => [["is", "event:goal", ["PurchaseUSD", "PurchaseEUR"]]],
+            "dimensions" => ["time:hour"]
+          })
+
+        assert %{"error" => error} = json_response(response, 400)
+        assert error =~ "Revenue metrics were dropped"
+      end
+
+      test "#{revenue_metric} with insufficient subscription", %{
+        conn: conn,
+        user: user,
+        site: site
+      } do
+        insert(:goal, site: site, event_name: "PurchaseEUR", currency: "EUR")
+        subscribe_to_growth_plan(user)
+
+        response =
+          do_query_fail(conn, site, %{
+            "date_range" => "day",
+            "metrics" => [unquote(revenue_metric)],
+            "filters" => [["is", "event:goal", ["PurchaseEUR"]]],
+            "dimensions" => ["time:hour"]
+          })
+
+        assert %{"error" => error} = json_response(response, 400)
+        assert error =~ "Revenue metrics were dropped"
+      end
+    end
+  end
+
   describe "total_revenue plot" do
     @describetag :ee_only
     setup [:create_user, :log_in, :create_site, :create_legacy_site_import]
@@ -2205,6 +2266,49 @@ defmodule PlausibleWeb.Api.StatsController.MainGraphTest do
                }
              ]
     end
+
+    test "silently ignores imported data", %{conn: conn, site: site} do
+      insert(:goal, site: site, event_name: "Payment", currency: "USD")
+
+      populate_stats(site, [
+        build(:event,
+          name: "Payment",
+          revenue_reporting_amount: Decimal.new("13.29"),
+          revenue_reporting_currency: "USD",
+          timestamp: ~N[2021-01-01 00:00:00]
+        ),
+        build(:event,
+          name: "Payment",
+          revenue_reporting_amount: Decimal.new("13.21"),
+          revenue_reporting_currency: "USD",
+          timestamp: ~N[2021-01-01 00:00:00]
+        ),
+        build(:imported_visitors, date: ~D[2021-01-01])
+      ])
+
+      response =
+        do_query(conn, site, %{
+          "date_range" => "all",
+          "metrics" => ["total_revenue"],
+          "dimensions" => ["time:month"],
+          "filters" => [["is", "event:goal", ["Payment"]]],
+          "include" => %{"imports" => true}
+        })
+
+      assert response["results"] == [
+               %{
+                 "dimensions" => ["2021-01-01"],
+                 "metrics" => [
+                   %{
+                     "currency" => "USD",
+                     "long" => "$26.50",
+                     "short" => "$26.5",
+                     "value" => 26.5
+                   }
+                 ]
+               }
+             ]
+    end
   end
 
   describe "average_revenue plot" do
@@ -2381,6 +2485,49 @@ defmodule PlausibleWeb.Api.StatsController.MainGraphTest do
                }
              ]
     end
+
+    test "silently ignores imported data", %{conn: conn, site: site} do
+      insert(:goal, site: site, event_name: "Payment", currency: "USD")
+
+      populate_stats(site, [
+        build(:event,
+          name: "Payment",
+          revenue_reporting_amount: Decimal.new("3.3"),
+          revenue_reporting_currency: "USD",
+          timestamp: ~N[2021-01-01 00:00:00]
+        ),
+        build(:event,
+          name: "Payment",
+          revenue_reporting_amount: Decimal.new("1.1"),
+          revenue_reporting_currency: "USD",
+          timestamp: ~N[2021-01-01 00:00:00]
+        ),
+        build(:imported_visitors, date: ~D[2021-01-01])
+      ])
+
+      response =
+        do_query(conn, site, %{
+          "date_range" => "all",
+          "metrics" => ["average_revenue"],
+          "dimensions" => ["time:month"],
+          "filters" => [["is", "event:goal", ["Payment"]]],
+          "include" => %{"imports" => true}
+        })
+
+      assert response["results"] == [
+               %{
+                 "dimensions" => ["2021-01-01"],
+                 "metrics" => [
+                   %{
+                     "currency" => "USD",
+                     "long" => "$2.20",
+                     "short" => "$2.2",
+                     "value" => 2.2
+                   }
+                 ]
+               }
+             ]
+    end
   end
 
   describe "present_index" do