Get rid of old /top-stats endpoint (#6176)

* transform top_stats_test.exs to target /query endpoint

* change test assertion according to new behaviour

With the switch to the /query endpoint, the aggregate query with a page
filter started returning `time_on_page` as `nil` without any engagement
data, instead of 0 as before. When this happens, Time on Page is simply
hidden in Top Stats.

* remove old /top-stats endpoint

* tag test ee only

* review feebdack

* format

* fix function signature

* add filters key

Author: RobertJoonas

lib/plausible_web/controllers/api/stats_controller.ex

@@ -12,7 +12,6 @@ defmodule PlausibleWeb.Api.StatsController do
     Filters,
     Time,
     TableDecider,
-    TimeOnPage,
     Dashboard,
     ParsedQueryParams,
     QueryBuilder,
@@ -207,56 +206,6 @@ defmodule PlausibleWeb.Api.StatsController do
     end
   end
 
-  def top_stats(conn, params) do
-    site = conn.assigns[:site]
-
-    params = realtime_period_to_30m(params)
-
-    query =
-      site
-      |> Query.from(params, debug_metadata: debug_metadata(conn))
-      |> Query.set_include(:imports_meta, true)
-
-    %{
-      top_stats: top_stats,
-      meta: meta,
-      sample_percent: sample_percent,
-      graphable_metrics: graphable_metrics
-    } = fetch_top_stats(site, query)
-
-    comparison_query = comparison_query(query)
-
-    json(conn, %{
-      top_stats: top_stats,
-      meta: meta,
-      graphable_metrics: graphable_metrics,
-      interval: query.interval,
-      sample_percent: sample_percent,
-      with_imported_switch: with_imported_switch_info(meta),
-      includes_imported: meta[:imports_included] == true,
-      comparing_from: query.include.compare && Query.date_range(comparison_query).first,
-      comparing_to: query.include.compare && Query.date_range(comparison_query).last,
-      from: Query.date_range(query).first,
-      to: Query.date_range(query).last
-    })
-  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} ->
-        %{visible: true, togglable: false, tooltip_msg: "Imported data cannot be included"}
-
-      {false, reason} when reason in [:no_imported_data, :out_of_range] ->
-        %{visible: false, togglable: false, tooltip_msg: nil}
-    end
-  end
-
   defp present_index_for(site, query, dates) do
     case query.interval do
       "hour" ->
@@ -303,197 +252,6 @@ defmodule PlausibleWeb.Api.StatsController do
     end
   end
 
-  defp fetch_top_stats(site, query) do
-    goal_filter? =
-      toplevel_goal_filter?(query)
-
-    cond do
-      query.input_date_range == :realtime_30m && goal_filter? ->
-        fetch_goal_realtime_top_stats(site, query)
-
-      query.input_date_range == :realtime_30m ->
-        fetch_realtime_top_stats(site, query)
-
-      goal_filter? ->
-        fetch_goal_top_stats(site, query)
-
-      true ->
-        fetch_other_top_stats(site, query)
-    end
-  end
-
-  defp fetch_goal_realtime_top_stats(site, query) do
-    query = Query.set_include(query, :compare, nil)
-
-    %{
-      results: %{
-        visitors: %{value: unique_conversions},
-        events: %{value: total_conversions}
-      },
-      meta: meta
-    } = Stats.aggregate(site, query, [:visitors, :events])
-
-    top_stats = [
-      %{
-        name: "Current visitors",
-        graph_metric: :current_visitors,
-        value: Stats.current_visitors(site)
-      },
-      %{
-        name: "Unique conversions (last 30 min)",
-        graph_metric: :visitors,
-        value: unique_conversions
-      },
-      %{
-        name: "Total conversions (last 30 min)",
-        graph_metric: :events,
-        value: total_conversions
-      }
-    ]
-
-    %{
-      top_stats: top_stats,
-      meta: meta,
-      graphable_metrics: [:visitors, :events],
-      sample_percent: 100
-    }
-  end
-
-  defp fetch_realtime_top_stats(site, query) do
-    query = Query.set_include(query, :compare, nil)
-
-    %{
-      results: %{
-        visitors: %{value: visitors},
-        pageviews: %{value: pageviews}
-      },
-      meta: meta
-    } = Stats.aggregate(site, query, [:visitors, :pageviews])
-
-    top_stats = [
-      %{
-        name: "Current visitors",
-        graph_metric: :current_visitors,
-        value: Stats.current_visitors(site)
-      },
-      %{
-        name: "Unique visitors (last 30 min)",
-        graph_metric: :visitors,
-        value: visitors
-      },
-      %{
-        name: "Pageviews (last 30 min)",
-        graph_metric: :pageviews,
-        value: pageviews
-      }
-    ]
-
-    %{
-      top_stats: top_stats,
-      meta: meta,
-      sample_percent: 100,
-      graphable_metrics: [:visitors, :pageviews]
-    }
-  end
-
-  defp fetch_goal_top_stats(site, query) do
-    metrics =
-      [:visitors, :events, :conversion_rate] ++ @revenue_metrics
-
-    %{results: results, meta: meta} = Stats.aggregate(site, query, metrics)
-
-    top_stats =
-      [
-        top_stats_entry(results, "Unique conversions", :visitors),
-        top_stats_entry(results, "Total conversions", :events),
-        on_ee do
-          top_stats_entry(results, "Average revenue", :average_revenue)
-        end,
-        on_ee do
-          top_stats_entry(results, "Total revenue", :total_revenue)
-        end,
-        top_stats_entry(results, "Conversion rate", :conversion_rate)
-      ]
-      |> Enum.reject(&is_nil/1)
-
-    %{top_stats: top_stats, meta: meta, graphable_metrics: metrics, sample_percent: 100}
-  end
-
-  defp fetch_other_top_stats(site, query) do
-    page_filter? =
-      Filters.filtering_on_dimension?(query, "event:page", behavioral_filters: :ignore)
-
-    metrics = [:visitors, :visits, :pageviews, :sample_percent]
-
-    metrics =
-      cond do
-        page_filter? and query.include_imported ->
-          metrics ++ [:bounce_rate, :scroll_depth]
-
-        page_filter? ->
-          metrics ++ [:bounce_rate, :scroll_depth, :time_on_page]
-
-        true ->
-          metrics ++ [:views_per_visit, :bounce_rate, :visit_duration]
-      end
-
-    %{results: results, meta: meta} = Stats.aggregate(site, query, metrics)
-
-    top_stats =
-      [
-        top_stats_entry(results, "Unique visitors", :visitors),
-        top_stats_entry(results, "Total visits", :visits),
-        top_stats_entry(results, "Total pageviews", :pageviews),
-        top_stats_entry(results, "Views per visit", :views_per_visit),
-        top_stats_entry(results, "Bounce rate", :bounce_rate),
-        top_stats_entry(results, "Visit duration", :visit_duration),
-        top_stats_entry(results, "Time on page", :time_on_page,
-          formatter: fn
-            nil -> 0
-            value -> value
-          end
-        ),
-        top_stats_entry(results, "Scroll depth", :scroll_depth)
-      ]
-      |> Enum.filter(& &1)
-
-    sample_percent = results[:sample_percent][:value]
-
-    %{
-      top_stats: top_stats,
-      meta: meta,
-      graphable_metrics:
-        if(TimeOnPage.new_time_on_page_visible?(site),
-          do: metrics,
-          else: metrics -- [:time_on_page]
-        ),
-      sample_percent: sample_percent
-    }
-  end
-
-  defp top_stats_entry(current_results, name, key, opts \\ []) do
-    if current_results[key] do
-      formatter = Keyword.get(opts, :formatter, & &1)
-      value = get_in(current_results, [key, :value])
-
-      %{name: name, value: formatter.(value), graph_metric: key}
-      |> maybe_put_comparison(current_results, key, formatter)
-    end
-  end
-
-  defp maybe_put_comparison(entry, results, key, formatter) do
-    prev_value = get_in(results, [key, :comparison_value])
-    change = get_in(results, [key, :change])
-
-    if prev_value do
-      entry
-      |> Map.put(:comparison_value, formatter.(prev_value))
-      |> Map.put(:change, change)
-    else
-      entry
-    end
-  end
-
   def sources(conn, params) do
     site = conn.assigns[:site]
     params = Map.put(params, "property", "visit:source")

lib/plausible_web/router.ex

@@ -286,7 +286,6 @@ defmodule PlausibleWeb.Router do
         post "/:domain/query", StatsController, :query
         get "/:domain/current-visitors", StatsController, :current_visitors
         get "/:domain/main-graph", StatsController, :main_graph
-        get "/:domain/top-stats", StatsController, :top_stats
         get "/:domain/sources", StatsController, :sources
         get "/:domain/channels", StatsController, :channels
         get "/:domain/utm_mediums", StatsController, :utm_mediums

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

@@ -1,6 +1,14 @@
 defmodule PlausibleWeb.Api.StatsController.AuthorizationTest do
   use PlausibleWeb.ConnCase, async: true
 
+  defp query_visitors(conn, site, params) do
+    params =
+      [date_range: "day", metrics: ["visitors"], filters: []]
+      |> Keyword.merge(params)
+
+    post(conn, Routes.stats_path(conn, :query, site.domain), params)
+  end
+
   describe "API authorization - as anonymous user" do
     test "returns 404 for a site that doesn't exist", %{conn: conn} do
       conn = init_session(conn)
@@ -34,7 +42,7 @@ defmodule PlausibleWeb.Api.StatsController.AuthorizationTest do
     test "returns 404 for non-existent shared link", %{conn: conn} do
       site = new_site()
 
-      conn = get(conn, "/api/stats/#{site.domain}/top-stats?auth=does-not-exist")
+      conn = query_visitors(conn, site, auth: "does-not-exist")
 
       assert json_response(conn, 404) == %{
                "error" => "Site does not exist or user does not have sufficient access."
@@ -45,9 +53,9 @@ defmodule PlausibleWeb.Api.StatsController.AuthorizationTest do
       site = new_site()
       link = insert(:shared_link, site: site)
 
-      conn = get(conn, "/api/stats/#{site.domain}/top-stats?auth=#{link.slug}")
+      conn = query_visitors(conn, site, auth: link.slug)
 
-      assert %{"top_stats" => _any} = json_response(conn, 200)
+      assert %{"results" => _any} = json_response(conn, 200)
     end
 
     test "returns 200 for password-protected link with valid cookie", %{conn: conn} do
@@ -62,9 +70,9 @@ defmodule PlausibleWeb.Api.StatsController.AuthorizationTest do
       conn =
         conn
         |> put_req_cookie(cookie_name, token)
-        |> get("/api/stats/#{site.domain}/top-stats?auth=#{link.slug}")
+        |> query_visitors(site, auth: link.slug)
 
-      assert %{"top_stats" => _any} = json_response(conn, 200)
+      assert %{"results" => _any} = json_response(conn, 200)
     end
 
     test "returns 404 for password-protected link with invalid cookie value", %{conn: conn} do
@@ -86,7 +94,7 @@ defmodule PlausibleWeb.Api.StatsController.AuthorizationTest do
       conn =
         conn
         |> put_req_cookie(cookie_name, other_link_token)
-        |> get("/api/stats/#{site.domain}/top-stats?auth=#{link.slug}")
+        |> query_visitors(site, auth: link.slug)
 
       assert json_response(conn, 404) == %{
                "error" => "Site does not exist or user does not have sufficient access."
@@ -99,7 +107,7 @@ defmodule PlausibleWeb.Api.StatsController.AuthorizationTest do
       link =
         insert(:shared_link, site: site, password_hash: Plausible.Auth.Password.hash("password"))
 
-      conn = get(conn, "/api/stats/#{site.domain}/top-stats?auth=#{link.slug}")
+      conn = query_visitors(conn, site, auth: link.slug)
 
       assert json_response(conn, 404) == %{
                "error" => "Site does not exist or user does not have sufficient access."
@@ -116,7 +124,7 @@ defmodule PlausibleWeb.Api.StatsController.AuthorizationTest do
       link =
         insert(:shared_link, site: site, segment: segment)
 
-      conn = get(conn, "/api/stats/#{site.domain}/top-stats?auth=#{link.slug}")
+      conn = query_visitors(conn, site, auth: link.slug)
 
       assert json_response(conn, 400) == %{
                "error" => "The first filter must be for the segment with id #{segment.id}"
@@ -133,9 +141,9 @@ defmodule PlausibleWeb.Api.StatsController.AuthorizationTest do
         insert(:shared_link, site: site, segment: segment)
 
       conn =
-        get(
-          conn,
-          "/api/stats/#{site.domain}/top-stats?auth=#{link.slug}&filters=#{JSON.encode!([["is", "segment", [segment.id + 1]]])}"
+        query_visitors(conn, site,
+          auth: link.slug,
+          filters: [["is", "segment", [segment.id + 1]]]
         )
 
       assert json_response(conn, 400) == %{
@@ -153,9 +161,12 @@ defmodule PlausibleWeb.Api.StatsController.AuthorizationTest do
         insert(:shared_link, site: site, segment: segment)
 
       conn =
-        get(
-          conn,
-          "/api/stats/#{site.domain}/top-stats?auth=#{link.slug}&filters=#{JSON.encode!([["is", "segment", [segment.id]], ["is", "event:page", ["/docs"]]])}"
+        query_visitors(conn, site,
+          auth: link.slug,
+          filters: [
+            ["is", "segment", [segment.id]],
+            ["is", "event:page", ["/docs"]]
+          ]
         )
 
       assert json_response(conn, 200)