Reject unknown imported cities from queries (#2675)

* Reject unknown imported cities from queries

This commit fixes a bug where the city report returned `N/A` entries.
The functions that build imported data queries were using SQL
`COALESCE`, assuming city data is `NULL` when unknown, when actually its
unknown value is `0`.

This commit addresses the problem using SQL `NULLIF` combined with the
previous `COALESCE` call. With this change both `0` and `NULL` are
treated as unknown.

Since 1cb07ef cities can be `NULL`, but
previously we saved `0` as unknown.

Closes #1960

* Add entry to CHANGELOG

* Ignore cyclomatic complexity Credo check

CHANGELOG.md

@@ -8,6 +8,7 @@ All notable changes to this project will be documented in this file.
 - Add support for more Bamboo adapters, i.e. `Bamboo.MailgunAdapter`, `Bamboo.MandrillAdapter`, `Bamboo.SendGridAdapter` plausible/analytics#2649
 
 ### Fixed
+- City report showing N/A instead of city names with imported data plausible/analytics#2675
 - Empty values for Screen Size, OS and Browser are uniformly replaced with "(not set)"
 - Fix [more pageviews with session prop filter than with no filters](https://github.com/plausible/analytics/issues/1666)
 - Cascade delete sent_renewal_notifications table when user is deleted plausible/analytics#2549

lib/plausible/stats/imported.ex

@@ -58,6 +58,7 @@ defmodule Plausible.Stats.Imported do
   # GA only has 'source'
   def merge_imported(q, _, _, "utm_source", _), do: q
 
+  # credo:disable-for-next-line Credo.Check.Refactor.CyclomaticComplexity
   def merge_imported(q, site, query, property, metrics)
       when property in [
              "visit:source",
@@ -195,7 +196,9 @@ defmodule Plausible.Stats.Imported do
           imported_q |> where([i], i.region != "") |> select_merge([i], %{region: i.region})
 
         :city ->
-          imported_q |> where([i], i.city != 0) |> select_merge([i], %{city: i.city})
+          imported_q
+          |> where([i], i.city != 0 and not is_nil(i.city))
+          |> select_merge([i], %{city: i.city})
 
         :device ->
           imported_q |> select_merge([i], %{device: i.device})
@@ -287,8 +290,8 @@ defmodule Plausible.Stats.Imported do
 
       :city ->
         q
-        |> select_merge([i, s], %{
-          city: fragment("coalesce(?, ?)", s.city, i.city)
+        |> select_merge([s, i], %{
+          city: fragment("coalesce(nullif(?, 0), ?)", i.city, s.city)
         })
 
       :device ->

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

@@ -31,5 +31,20 @@ defmodule PlausibleWeb.Api.StatsController.CitiesTest do
                %{"code" => 591_632, "country_flag" => "🇪🇪", "name" => "Kärdla", "visitors" => 2}
              ]
     end
+
+    test "does not return missing cities from imported data", %{conn: conn, site: site} do
+      populate_stats(site, [
+        build(:imported_locations, country: "EE", region: "EE-37", city: 588_409),
+        build(:imported_locations, country: nil, region: nil, city: 0),
+        build(:imported_locations, country: nil, region: nil, city: nil)
+      ])
+
+      conn = get(conn, "/api/stats/#{site.domain}/cities?period=day&with_imported=true")
+
+      assert json_response(conn, 200) == [
+               %{"code" => 588_409, "country_flag" => "🇪🇪", "name" => "Tallinn", "visitors" => 4},
+               %{"code" => 591_632, "country_flag" => "🇪🇪", "name" => "Kärdla", "visitors" => 2}
+             ]
+    end
   end
 end