Skip to content

Conversation

macobo
Copy link
Contributor

@macobo macobo commented Jan 15, 2024

Changes

This PR a filtering with contains/matches for custom properties, e.g. "Filter where (custom property) url contains foo"

image

First PR 🙌

Tests

  • Automated tests have been added
  • This PR does not require tests

Changelog

  • Entry has been added to changelog
  • This PR does not make a user-facing change

Documentation

  • Docs have been updated
  • This change does not need a documentation update

Dark mode

  • The UI has been tested both in dark and light mode
  • This PR does not change the UI

@macobo macobo requested a review from a team January 15, 2024 13:20
@macobo macobo force-pushed the custom-props-matches-support branch from 5c3d1d9 to e086e6d Compare January 15, 2024 13:21
@aerosol
Copy link
Member

aerosol commented Jan 15, 2024

There's something wrong with re-applying the filter when modifying the custom props breakdown query:

record-2024-01-15-16-45-30-acre.putt.blot.mp4

@aerosol
Copy link
Member

aerosol commented Jan 15, 2024

On a freshly seeded db (you can repro via mix ecto.reset), Total Visits doubles when a non-existing property value is supplied to the query:

record-2024-01-15-16-49-48-hull.roth.john.mp4

I thought it was due to seed data being nonsense, but modifying the test you've added, also returns extra rows that shouldn't be included?

diff --git a/test/plausible_web/controllers/api/stats_controller/pages_test.exs b/test/plausible_web/controllers/api/stats_controller/pages_test.exs
index 339a4bab3..ba9aa0fed 100644
--- a/test/plausible_web/controllers/api/stats_controller/pages_test.exs
+++ b/test/plausible_web/controllers/api/stats_controller/pages_test.exs
@@ -103,7 +103,7 @@ defmodule PlausibleWeb.Api.StatsController.PagesTest do
         build(:pageview, pathname: "/5")
       ])
 
-      filters = Jason.encode!(%{props: %{"prop" => "~bar"}})
+      filters = Jason.encode!(%{props: %{"prop" => "~bar|pppp"}})
       conn = get(conn, "/api/stats/#{site.domain}/pages?period=day&filters=#{filters}")
 
       assert json_response(conn, 200) == [

@macobo macobo force-pushed the custom-props-matches-support branch from e086e6d to 43772bc Compare January 18, 2024 19:44
@macobo
Copy link
Contributor Author

macobo commented Jan 18, 2024

@aerosol thank you for the great feedback. I've updated the branch.

(1) Regarding breakdowns, this was a legit bug in the code, see fix in 43772bc

(2) seems to be about how regex is internally escaped. I hoped that merging #3634 would fix this, but it did not. Given that this behavior already exists on master, I'll fix this in a separate PR, filed it in my worklog.

@macobo macobo mentioned this pull request Jan 22, 2024
@macobo macobo merged commit 1b95433 into master Jan 22, 2024
@macobo macobo deleted the custom-props-matches-support branch January 22, 2024 10:24
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.

2 participants