APIv2: Allow sending numbers for visit:city along with strings (#4313)

* Allow sending numbers for visit:city along with strings

This simplifies querying as we pass the list of cities as numbers to clients

* use &is_integer/1

* Remove redundancy

assets/js/dashboard/util/filters.js

@@ -147,7 +147,6 @@ export function serializeApiFilters(filters) {
     if (filterKey.startsWith(EVENT_PROPS_PREFIX) || EVENT_FILTER_KEYS.has(filterKey)) {
       apiFilterKey = `event:${filterKey}`
     }
-    clauses = clauses.map((value) => value.toString())
     return [operation, apiFilterKey, clauses]
   })
 

lib/plausible/stats/filters/query_parser.ex

@@ -92,11 +92,13 @@ defmodule Plausible.Stats.Filters.QueryParser do
 
   defp parse_clauses_list([_operation, filter_key, list] = filter) when is_list(list) do
     all_strings? = Enum.all?(list, &is_binary/1)
+    all_integers? = Enum.all?(list, &is_integer/1)
 
-    cond do
-      filter_key == "event:goal" && all_strings? -> {:ok, [Filters.Utils.wrap_goal_value(list)]}
-      filter_key != "event:goal" && all_strings? -> {:ok, [list]}
-      true -> {:error, "Invalid filter '#{inspect(filter)}'"}
+    case {filter_key, all_strings?, all_integers?} do
+      {"event:goal", true, _} -> {:ok, [Filters.Utils.wrap_goal_value(list)]}
+      {"visit:city", _, true} -> {:ok, [list]}
+      {_, true, _} -> {:ok, [list]}
+      _ -> {:error, "Invalid filter '#{inspect(filter)}'"}
     end
   end
 

test/plausible/stats/query_parser_test.exs

@@ -245,6 +245,53 @@ defmodule Plausible.Stats.Filters.QueryParserTest do
       }
       |> check_error(site, ~r/Invalid filters passed/)
     end
+
+    test "numeric filter is invalid", %{site: site} do
+      %{
+        "metrics" => ["visitors"],
+        "date_range" => "all",
+        "filters" => [["is", "visit:os_version", [123]]]
+      }
+      |> check_error(site, ~r/Invalid filter /)
+    end
+
+    test "numbers and strings are valid for visit:city", %{site: site} do
+      %{
+        "metrics" => ["visitors"],
+        "date_range" => "all",
+        "filters" => [["is", "visit:city", [123, 456]]]
+      }
+      |> check_success(site, %{
+        metrics: [:visitors],
+        date_range: @date_range,
+        filters: [
+          [:is, "visit:city", [123, 456]]
+        ],
+        dimensions: [],
+        order_by: nil,
+        timezone: site.timezone,
+        include: %{imports: false, time_labels: false},
+        preloaded_goals: []
+      })
+
+      %{
+        "metrics" => ["visitors"],
+        "date_range" => "all",
+        "filters" => [["is", "visit:city", ["123", "456"]]]
+      }
+      |> check_success(site, %{
+        metrics: [:visitors],
+        date_range: @date_range,
+        filters: [
+          [:is, "visit:city", ["123", "456"]]
+        ],
+        dimensions: [],
+        order_by: nil,
+        timezone: site.timezone,
+        include: %{imports: false, time_labels: false},
+        preloaded_goals: []
+      })
+    end
   end
 
   describe "include validation" do

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

@@ -834,6 +834,28 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryTest do
 
       assert json_response(conn, 200)["results"] == [%{"metrics" => [3], "dimensions" => []}]
     end
+
+    test "handles filtering by visit:city", %{
+      conn: conn,
+      site: site
+    } do
+      populate_stats(site, [
+        build(:pageview, city_geoname_id: 588_409),
+        build(:pageview, city_geoname_id: 689_123),
+        build(:pageview, city_geoname_id: 0),
+        build(:pageview, city_geoname_id: 10)
+      ])
+
+      conn =
+        post(conn, "/api/v2/query", %{
+          "site_id" => site.domain,
+          "date_range" => "all",
+          "metrics" => ["pageviews"],
+          "filters" => [["is", "visit:city", [588_409, 689_123]]]
+        })
+
+      assert json_response(conn, 200)["results"] == [%{"metrics" => [2], "dimensions" => []}]
+    end
   end
 
   describe "timeseries" do