Props details view (#3196)

* make (none) value in custom prop breakdown add +1 to pagination limit

This is needed because the (none) value is always added to the
breakdown_results **after** fetching those from ClickHouse.

* only include (none) values on the first page of results

* fix percentage metric calculation for paginated results

Instead of summing up the number of visitors from the breakdown results
to get the total, we have to make a separate query to `Stats.aggregate`.
Otherwise, the percentages for each results page will wrongly add up to 100%.

Since imported data for aggregated visitors and other properties (such as
browsers, OSs, etc) live in different tables, we have to tweak the tests to
also include the same number of visitors in the `imported_visitors` table.

* add details view for props

* changelog

* exclude imported data from total

CHANGELOG.md

@@ -4,6 +4,7 @@ All notable changes to this project will be documented in this file.
 ## Unreleased
 
 ### Fixed
+- Only return `(none)` values in custom property breakdown for the first page (pagination) of results
 - Fixed weekly/monthly e-mail report [rendering issues](https://github.com/plausible/analytics/issues/284)
 - Fixed [IPv6 problems](https://github.com/plausible/analytics/issues/3173) in data migration plausible/analytics#3179
 - Fixed [long URLs display](https://github.com/plausible/analytics/issues/3158) in Outbound Link breakdown view

assets/js/dashboard/router.js

@@ -9,7 +9,9 @@ import PagesModal from './stats/modals/pages'
 import EntryPagesModal from './stats/modals/entry-pages'
 import ExitPagesModal from './stats/modals/exit-pages'
 import ModalTable from './stats/modals/table'
+import PropsModal from './stats/modals/props'
 import FilterModal from './stats/modals/filter-modal'
+import * as url from './util/url';
 
 function ScrollToTop() {
   const location = useLocation();
@@ -49,13 +51,16 @@ export default function Router({site, loggedIn, currentUserRole}) {
             <ExitPagesModal site={site} />
           </Route>
           <Route path="/:domain/countries">
-            <ModalTable title="Top countries" site={site} endpoint={`/api/stats/${encodeURIComponent(site.domain)}/countries`} filter={{country: 'code', country_name: 'name'}} keyLabel="Country" renderIcon={renderCountryIcon} />
+            <ModalTable title="Top countries" site={site} endpoint={url.apiPath(site, '/countries')} filter={{country: 'code', country_name: 'name'}} keyLabel="Country" renderIcon={renderCountryIcon} />
           </Route>
           <Route path="/:domain/regions">
-            <ModalTable title="Top regions" site={site} endpoint={`/api/stats/${encodeURIComponent(site.domain)}/regions`} filter={{region: 'code', region_name: 'name'}} keyLabel="Region" renderIcon={renderRegionIcon} />
+            <ModalTable title="Top regions" site={site} endpoint={url.apiPath(site, '/regions')} filter={{region: 'code', region_name: 'name'}} keyLabel="Region" renderIcon={renderRegionIcon} />
           </Route>
           <Route path="/:domain/cities">
-            <ModalTable title="Top cities" site={site} endpoint={`/api/stats/${encodeURIComponent(site.domain)}/cities`} filter={{city: 'code', city_name: 'name'}} keyLabel="City" renderIcon={renderCityIcon} />
+            <ModalTable title="Top cities" site={site} endpoint={url.apiPath(site, '/cities')} filter={{city: 'code', city_name: 'name'}} keyLabel="City" renderIcon={renderCityIcon} />
+          </Route>
+          <Route path="/:domain/custom-prop-values/:prop_key">
+            <PropsModal site={site}/>
           </Route>
           <Route path={["/:domain/filter/:field"]}>
             <FilterModal site={site} />

assets/js/dashboard/stats/behaviours/props.js

@@ -17,7 +17,7 @@ export default function Properties(props) {
     return null
   }
 
-  function fetchProps() {    
+  function fetchProps() {
     return api.get(url.apiPath(site, `/custom-prop-values/${encodeURIComponent(propKey)}`), query)
   }
 
@@ -47,6 +47,7 @@ export default function Properties(props) {
           {name: 'events', label: 'Events'},
           query.filters.goal ? CR_METRIC : PERCENTAGE_METRIC
         ]}
+        detailsLink={url.sitePath(site, `/custom-prop-values/${propKey}`)}
         query={query}
         color="bg-red-50"
         colMinWidth={90}

assets/js/dashboard/stats/modals/props.js

@@ -0,0 +1,112 @@
+import React, { useEffect, useState } from "react";
+import { Link } from 'react-router-dom'
+import { withRouter } from 'react-router-dom'
+
+import Modal from './modal'
+import * as api from '../../api'
+import * as url from "../../util/url";
+import numberFormatter from '../../util/number-formatter'
+import {parseQuery} from '../../query'
+
+function PropsModal(props) {
+  const site = props.site
+  const query = parseQuery(props.location.search, site)
+  const propKey = props.location.pathname.split('/').pop()
+
+  const [loading, setLoading] = useState(true)
+  const [moreResultsAvailable, setMoreResultsAvailable] = useState(false)
+  const [page, setPage] = useState(1)
+  const [list, setList] = useState([])
+
+  useEffect(() => {
+    fetchData()
+  }, [])
+
+  function fetchData() {
+    api.get(url.apiPath(site, `/custom-prop-values/${propKey}`), query, {limit: 100, page})
+      .then((res) => {
+        setLoading(false)
+        setList(list.concat(res))
+        setPage(page + 1)
+        setMoreResultsAvailable(res.length >= 100)
+      })
+  }
+
+  function loadMore() {
+    setLoading(true)
+    fetchData()
+  }
+
+  function renderLoadMore() {
+    return (
+      <div className="w-full text-center my-4">
+        <button onClick={loadMore} type="button" className="button">
+          Load more
+        </button>
+      </div>
+    )
+  }
+
+  function filterSearchLink(listItem) {
+    const searchParams = new URLSearchParams(window.location.search)
+    searchParams.set('props', JSON.stringify({[propKey]: listItem['name']}))
+    return searchParams.toString()
+  }
+
+  function renderListItem(listItem) {
+    return (
+      <tr className="text-sm dark:text-gray-200" key={listItem.name}>
+        <td className="p-2">
+          <Link
+            to={{pathname: url.siteBasePath(site), search: filterSearchLink(listItem)}}
+            className="hover:underline block truncate">
+              {listItem.name}
+          </Link>
+        </td>
+        <td className="p-2 w-32 font-medium" align="right">{numberFormatter(listItem.visitors)}</td>
+        <td className="p-2 w-32 font-medium" align="right">{numberFormatter(listItem.events)}</td>
+        { query.filters.goal && <td className="p-2 w-32 font-medium" align="right">{listItem.conversion_rate}%</td> }
+        { !query.filters.goal && <td className="p-2 w-32 font-medium" align="right">{listItem.percentage}</td> }
+      </tr>
+    )
+  }
+
+  function renderLoading() {
+    return <div className="loading my-16 mx-auto"><div></div></div>
+  }
+
+  function renderBody() {
+    return (
+      <>
+        <h1 className="text-xl font-bold dark:text-gray-100">Custom Property breakdown</h1>
+
+        <div className="my-4 border-b border-gray-300"></div>
+        <main className="modal__content">
+          <table className="w-max overflow-x-auto md:w-full table-striped table-fixed">
+            <thead>
+              <tr>
+                <th className="p-2 w-48 md:w-56 lg:w-1/3 text-xs tracking-wide font-bold text-gray-500 dark:text-gray-400 truncate" align="left">{propKey}</th>
+                <th className="p-2 w-32 text-xs tracking-wide font-bold text-gray-500 dark:text-gray-400" align="right">Visitors</th>
+                <th className="p-2 w-32 text-xs tracking-wide font-bold text-gray-500 dark:text-gray-400" align="right">Events</th>
+                <th className="p-2 w-32 text-xs tracking-wide font-bold text-gray-500 dark:text-gray-400" align="right">{query.filters.goal ? 'CR' : '%'}</th>
+              </tr>
+            </thead>
+            <tbody>
+              { list.map(renderListItem) }
+            </tbody>
+          </table>
+        </main>
+      </>
+    )
+  }
+
+  return (
+    <Modal site={site}>
+      { renderBody() }
+      { loading && renderLoading() }
+      { !loading && moreResultsAvailable && renderLoadMore() }
+    </Modal>
+  )
+}
+
+export default withRouter(PropsModal)

assets/js/dashboard/stats/reports/list.js

@@ -174,7 +174,7 @@ export default function ListReport(props) {
   function renderReportBody() {
     return (
       <FlipMove className="flex-grow">
-        {state.list.map(renderRow)}
+        {state.list.slice(0, MAX_ITEMS).map(renderRow)}
       </FlipMove>
     )
   }

lib/plausible/stats/breakdown.ex

@@ -68,11 +68,11 @@ defmodule Plausible.Stats.Breakdown do
   end
 
   def breakdown(site, query, "event:props:" <> custom_prop = property, metrics, pagination) do
-    {limit, _} = pagination
     {currency, metrics} = get_revenue_tracking_currency(site, query, metrics)
+    {_limit, page} = pagination
 
     none_result =
-      if include_none_result?(query.filters[property]) do
+      if page == 1 && include_none_result?(query.filters[property]) do
         none_query = Query.put_filter(query, property, {:is, "(none)"})
 
         from(e in base_event_query(site, none_query),
@@ -88,17 +88,10 @@ defmodule Plausible.Stats.Breakdown do
 
     trace(query, property, metrics)
 
-    results =
+    breakdown_events(site, query, "event:props:" <> custom_prop, metrics, pagination)
-      breakdown_events(site, query, "event:props:" <> custom_prop, metrics, pagination)
+    |> Kernel.++(none_result)
-      |> Kernel.++(none_result)
+    |> Enum.map(&cast_revenue_metrics_to_money(&1, currency))
-      |> Enum.map(&cast_revenue_metrics_to_money(&1, currency))
+    |> Enum.sort_by(& &1[sorting_key(metrics)], :desc)
-      |> Enum.sort_by(& &1[sorting_key(metrics)], :desc)
-
-    if Enum.find_index(results, fn value -> value[custom_prop] == "(none)" end) == limit do
-      Enum.slice(results, 0..(limit - 1))
-    else
-      results
-    end
   end
 
   def breakdown(site, query, "event:page" = property, metrics, pagination) do

lib/plausible_web/controllers/api/stats_controller.ex

@@ -879,7 +879,7 @@ defmodule PlausibleWeb.Api.StatsController do
       Stats.breakdown(site, query, "visit:country", [:visitors], pagination)
       |> add_cr(site, query, {300, 1}, :country, "visit:country")
       |> transform_keys(%{country: :code})
-      |> add_percentages(query)
+      |> add_percentages(site, query)
 
     if params["csv"] do
       countries =
@@ -1002,7 +1002,7 @@ defmodule PlausibleWeb.Api.StatsController do
       Stats.breakdown(site, query, "visit:browser", [:visitors], pagination)
       |> add_cr(site, query, pagination, :browser, "visit:browser")
       |> transform_keys(%{browser: :name})
-      |> add_percentages(query)
+      |> add_percentages(site, query)
 
     if params["csv"] do
       if Map.has_key?(query.filters, "event:goal") do
@@ -1026,7 +1026,7 @@ defmodule PlausibleWeb.Api.StatsController do
       Stats.breakdown(site, query, "visit:browser_version", [:visitors], pagination)
       |> add_cr(site, query, pagination, :browser_version, "visit:browser_version")
       |> transform_keys(%{browser_version: :name})
-      |> add_percentages(query)
+      |> add_percentages(site, query)
 
     json(conn, versions)
   end
@@ -1040,7 +1040,7 @@ defmodule PlausibleWeb.Api.StatsController do
       Stats.breakdown(site, query, "visit:os", [:visitors], pagination)
       |> add_cr(site, query, pagination, :os, "visit:os")
       |> transform_keys(%{os: :name})
-      |> add_percentages(query)
+      |> add_percentages(site, query)
 
     if params["csv"] do
       if Map.has_key?(query.filters, "event:goal") do
@@ -1064,7 +1064,7 @@ defmodule PlausibleWeb.Api.StatsController do
       Stats.breakdown(site, query, "visit:os_version", [:visitors], pagination)
       |> add_cr(site, query, pagination, :os_version, "visit:os_version")
       |> transform_keys(%{os_version: :name})
-      |> add_percentages(query)
+      |> add_percentages(site, query)
 
     json(conn, versions)
   end
@@ -1078,7 +1078,7 @@ defmodule PlausibleWeb.Api.StatsController do
       Stats.breakdown(site, query, "visit:device", [:visitors], pagination)
       |> add_cr(site, query, pagination, :device, "visit:device")
       |> transform_keys(%{device: :name})
-      |> add_percentages(query)
+      |> add_percentages(site, query)
 
     if params["csv"] do
       if Map.has_key?(query.filters, "event:goal") do
@@ -1194,21 +1194,25 @@ defmodule PlausibleWeb.Api.StatsController do
   end
 
   defp breakdown_custom_prop_values(site, %{"prop_key" => prop_key} = params) do
-    query = Query.from(site, params) |> Filters.add_prefix()
+    query =
+      Query.from(site, params)
+      |> Filters.add_prefix()
+      |> Map.put(:include_imported, false)
+
     pagination = parse_pagination(params)
 
-    total_q = Query.remove_event_filters(query, [:goal, :props])
-
-    %{:visitors => %{value: total_unique_visitors}} = Stats.aggregate(site, total_q, [:visitors])
-
     prefixed_prop = "event:props:" <> prop_key
 
     props =
       Stats.breakdown(site, query, prefixed_prop, [:visitors, :events], pagination)
       |> transform_keys(%{prop_key => :name})
-      |> add_percentages(query)
+      |> add_percentages(site, query)
 
     if Map.has_key?(query.filters, "event:goal") do
+      total_q = Query.remove_event_filters(query, [:goal, :props])
+
+      %{visitors: %{value: total_unique_visitors}} = Stats.aggregate(site, total_q, [:visitors])
+
       Enum.map(props, fn prop ->
         Map.put(prop, :conversion_rate, calculate_cr(total_unique_visitors, prop.visitors))
       end)
@@ -1322,20 +1326,18 @@ defmodule PlausibleWeb.Api.StatsController do
 
   defp to_int(_, default), do: default
 
-  defp add_percentages([_ | _] = breakdown_result, query)
+  defp add_percentages([_ | _] = breakdown_result, site, query)
        when not is_map_key(query.filters, "event:goal") do
-    total = Enum.reduce(breakdown_result, 0, fn %{visitors: count}, total -> total + count end)
+    %{visitors: %{value: total_visitors}} = Stats.aggregate(site, query, [:visitors])
-    do_add_percentages(breakdown_result, total)
-  end
 
-  defp add_percentages(breakdown_result, _), do: breakdown_result
+    breakdown_result
-
+    |> Enum.map(fn stat ->
-  defp do_add_percentages(stat_list, total) do
+      Map.put(stat, :percentage, Float.round(stat.visitors / total_visitors * 100, 1))
-    Enum.map(stat_list, fn stat ->
-      Map.put(stat, :percentage, Float.round(stat.visitors / total * 100, 1))
     end)
   end
 
+  defp add_percentages(breakdown_result, _, _), do: breakdown_result
+
   defp add_cr([_ | _] = breakdown_results, site, query, pagination, key_name, filter_name)
        when is_map_key(query.filters, "event:goal") do
     items = Enum.map(breakdown_results, fn item -> Map.fetch!(item, key_name) end)

test/plausible/imported/imported_test.exs

@@ -837,7 +837,8 @@ defmodule Plausible.ImportedTest do
         build(:pageview,
           country_code: "GB",
           timestamp: ~N[2021-01-01 00:15:00]
-        )
+        ),
+        build(:imported_visitors, date: ~D[2021-01-01], visitors: 2)
       ])
 
       import_data(
@@ -905,7 +906,8 @@ defmodule Plausible.ImportedTest do
       populate_stats(site, [
         build(:pageview, screen_size: "Desktop", timestamp: ~N[2021-01-01 00:15:00]),
         build(:pageview, screen_size: "Desktop", timestamp: ~N[2021-01-01 00:15:00]),
-        build(:pageview, screen_size: "Laptop", timestamp: ~N[2021-01-01 00:15:00])
+        build(:pageview, screen_size: "Laptop", timestamp: ~N[2021-01-01 00:15:00]),
+        build(:imported_visitors, date: ~D[2021-01-01], visitors: 2)
       ])
 
       import_data(
@@ -949,7 +951,8 @@ defmodule Plausible.ImportedTest do
     test "Browsers data imported from Google Analytics", %{conn: conn, site: site} do
       populate_stats(site, [
         build(:pageview, browser: "Chrome", timestamp: ~N[2021-01-01 00:15:00]),
-        build(:pageview, browser: "Firefox", timestamp: ~N[2021-01-01 00:15:00])
+        build(:pageview, browser: "Firefox", timestamp: ~N[2021-01-01 00:15:00]),
+        build(:imported_visitors, visitors: 2, date: ~D[2021-01-01])
       ])
 
       import_data(
@@ -997,7 +1000,8 @@ defmodule Plausible.ImportedTest do
       populate_stats(site, [
         build(:pageview, operating_system: "Mac", timestamp: ~N[2021-01-01 00:15:00]),
         build(:pageview, operating_system: "Mac", timestamp: ~N[2021-01-01 00:15:00]),
-        build(:pageview, operating_system: "GNU/Linux", timestamp: ~N[2021-01-01 00:15:00])
+        build(:pageview, operating_system: "GNU/Linux", timestamp: ~N[2021-01-01 00:15:00]),
+        build(:imported_visitors, date: ~D[2021-01-01], visitors: 2)
       ])
 
       import_data(

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

@@ -113,7 +113,8 @@ defmodule PlausibleWeb.Api.StatsController.BrowsersTest do
       populate_stats(site, [
         build(:pageview, browser: "Chrome"),
         build(:imported_browsers, browser: "Chrome"),
-        build(:imported_browsers, browser: "Firefox")
+        build(:imported_browsers, browser: "Firefox"),
+        build(:imported_visitors, visitors: 2)
       ])
 
       conn = get(conn, "/api/stats/#{site.domain}/browsers?period=day")

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

@@ -6,21 +6,12 @@ defmodule PlausibleWeb.Api.StatsController.CountriesTest do
 
     test "returns top countries by new visitors", %{conn: conn, site: site} do
       populate_stats(site, [
-        build(:pageview,
+        build(:pageview, country_code: "EE"),
-          country_code: "EE"
+        build(:pageview, country_code: "EE"),
-        ),
+        build(:pageview, country_code: "GB"),
-        build(:pageview,
+        build(:imported_locations, country: "EE"),
-          country_code: "EE"
+        build(:imported_locations, country: "GB"),
-        ),
+        build(:imported_visitors, visitors: 2)
-        build(:pageview,
-          country_code: "GB"
-        ),
-        build(:imported_locations,
-          country: "EE"
-        ),
-        build(:imported_locations,
-          country: "GB"
-        )
       ])
 
       conn = get(conn, "/api/stats/#{site.domain}/countries?period=day")

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

@@ -2,7 +2,7 @@ defmodule PlausibleWeb.Api.StatsController.CustomPropBreakdownTest do
   use PlausibleWeb.ConnCase
 
   describe "GET /api/stats/:domain/custom-prop-values/:prop_key" do
-    setup [:create_user, :log_in, :create_new_site]
+    setup [:create_user, :log_in, :create_new_site, :add_imported_data]
 
     test "returns breakdown by a custom property", %{conn: conn, site: site} do
       prop_key = "parim_s6ber"
@@ -43,6 +43,30 @@ defmodule PlausibleWeb.Api.StatsController.CustomPropBreakdownTest do
              ]
     end
 
+    test "ignores imported data when calculating percentage", %{conn: conn, site: site} do
+      prop_key = "parim_s6ber"
+
+      populate_stats(site, [
+        build(:pageview, "meta.key": [prop_key], "meta.value": ["K2sna Kalle"]),
+        build(:imported_visitors, visitors: 2)
+      ])
+
+      conn =
+        get(
+          conn,
+          "/api/stats/#{site.domain}/custom-prop-values/#{prop_key}?period=day&with_imported=true"
+        )
+
+      assert json_response(conn, 200) == [
+               %{
+                 "visitors" => 1,
+                 "name" => "K2sna Kalle",
+                 "events" => 1,
+                 "percentage" => 100.0
+               }
+             ]
+    end
+
     test "returns (none) values in the breakdown", %{conn: conn, site: site} do
       prop_key = "parim_s6ber"
 
@@ -73,6 +97,86 @@ defmodule PlausibleWeb.Api.StatsController.CustomPropBreakdownTest do
                }
              ]
     end
+
+    test "(none) value is added as +1 to pagination limit", %{conn: conn, site: site} do
+      prop_key = "parim_s6ber"
+
+      populate_stats(site, [
+        build(:pageview, "meta.key": [prop_key], "meta.value": ["K2sna Kalle"]),
+        build(:pageview, "meta.key": [prop_key], "meta.value": ["K2sna Kalle"]),
+        build(:pageview)
+      ])
+
+      conn =
+        get(
+          conn,
+          "/api/stats/#{site.domain}/custom-prop-values/#{prop_key}?period=day&limit=1"
+        )
+
+      assert json_response(conn, 200) == [
+               %{
+                 "visitors" => 2,
+                 "name" => "K2sna Kalle",
+                 "events" => 2,
+                 "percentage" => 66.7
+               },
+               %{
+                 "visitors" => 1,
+                 "name" => "(none)",
+                 "events" => 1,
+                 "percentage" => 33.3
+               }
+             ]
+    end
+
+    test "(none) value is only included on the first page of results", %{conn: conn, site: site} do
+      prop_key = "kaksik"
+
+      populate_stats(site, [
+        build(:pageview, "meta.key": [prop_key], "meta.value": ["Teet"]),
+        build(:pageview, "meta.key": [prop_key], "meta.value": ["Teet"]),
+        build(:pageview, "meta.key": [prop_key], "meta.value": ["Tiit"]),
+        build(:pageview, "meta.key": [prop_key], "meta.value": ["Tiit"]),
+        build(:pageview, "meta.key": [prop_key], "meta.value": ["Tiit"]),
+        build(:pageview)
+      ])
+
+      conn1 =
+        get(
+          conn,
+          "/api/stats/#{site.domain}/custom-prop-values/#{prop_key}?period=day&limit=1&page=1"
+        )
+
+      conn2 =
+        get(
+          conn,
+          "/api/stats/#{site.domain}/custom-prop-values/#{prop_key}?period=day&limit=1&page=2"
+        )
+
+      assert json_response(conn1, 200) == [
+               %{
+                 "visitors" => 3,
+                 "name" => "Tiit",
+                 "events" => 3,
+                 "percentage" => 50.0
+               },
+               %{
+                 "visitors" => 1,
+                 "name" => "(none)",
+                 "events" => 1,
+                 "percentage" => 16.7
+               }
+             ]
+
+      assert json_response(conn2, 200) == [
+               %{
+                 "visitors" => 2,
+                 "name" => "Teet",
+                 "events" => 2,
+                 "percentage" => 33.3
+               }
+             ]
+    end
   end
 
   describe "GET /api/stats/:domain/custom-prop-values/:prop_key - with goal filter" do

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

@@ -150,7 +150,8 @@ defmodule PlausibleWeb.Api.StatsController.OperatingSystemsTest do
         build(:pageview, operating_system: "Mac"),
         build(:pageview, operating_system: "Android"),
         build(:imported_operating_systems, operating_system: "Mac"),
-        build(:imported_operating_systems, operating_system: "Android")
+        build(:imported_operating_systems, operating_system: "Android"),
+        build(:imported_visitors, visitors: 2)
       ])
 
       conn = get(conn, "/api/stats/#{site.domain}/operating-systems?period=day")

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

@@ -124,7 +124,8 @@ defmodule PlausibleWeb.Api.StatsController.ScreenSizesTest do
 
       populate_stats(site, [
         build(:imported_devices, device: "Mobile"),
-        build(:imported_devices, device: "Laptop")
+        build(:imported_devices, device: "Laptop"),
+        build(:imported_visitors, visitors: 2)
       ])
 
       conn = get(conn, "/api/stats/#{site.domain}/screen-sizes?period=day")