Skip to content

Reject unknown imported cities from queries - #2675

Merged
vinibrsl merged 3 commits into
masterfrom
reject-null-imported-cities
Feb 15, 2023
Merged

Reject unknown imported cities from queries#2675
vinibrsl merged 3 commits into
masterfrom
reject-null-imported-cities

Conversation

@vinibrsl

Copy link
Copy Markdown
Contributor

Changes

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

Tests

  • Automated tests have been added

Changelog

  • Entry has been added to changelog

Documentation

  • This change does not need a documentation update

Dark mode

  • This PR does not change the UI

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
@bundlemon

bundlemon Bot commented Feb 14, 2023

Copy link
Copy Markdown

BundleMon

Unchanged files (7)
Status Path Size Limits
static/css/app.css
515.02KB -
static/js/dashboard.js
298.61KB -
static/js/app.js
12.13KB -
static/js/embed.host.js
5.58KB -
static/js/embed.content.js
5.06KB -
tracker/js/plausible.js
748B -
static/js/applyTheme.js
314B -

No change in files bundle size

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

@vinibrsl
vinibrsl requested a review from a team February 14, 2023 19:39
q
|> select_merge([i, s], %{
city: fragment("coalesce(?, ?)", s.city, i.city)
|> select_merge([s, i], %{

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bindings were in incorrect order :thisisfine:

@vinibrsl
vinibrsl force-pushed the reject-null-imported-cities branch from cdb61dd to a4097c4 Compare February 14, 2023 19:48
@vinibrsl
vinibrsl merged commit 535874b into master Feb 15, 2023
@vinibrsl
vinibrsl deleted the reject-null-imported-cities branch February 15, 2023 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

N/A instead of city names on All Time view with imported GA data

2 participants