Support order_by in URL params of breakdown endpoints #4502
Conversation
| test "shows sources for a page (using new filters)", %{conn: conn, site: site} do | ||
| populate_stats(site, [ | ||
| build(:pageview, pathname: "/page1", referrer_source: "Google"), | ||
| build(:pageview, pathname: "/page1", referrer_source: "Google"), | ||
| build(:pageview, | ||
| user_id: 1, | ||
| pathname: "/page2", | ||
| referrer_source: "DuckDuckGo" | ||
| ), | ||
| build(:pageview, | ||
| user_id: 1, | ||
| pathname: "/page1", | ||
| referrer_source: "DuckDuckGo" | ||
| ) | ||
| ]) | ||
|
|
||
| filters = Jason.encode!([["is", "event:page", ["/page1"]]]) | ||
| conn = get(conn, "/api/stats/#{site.domain}/sources?filters=#{filters}") | ||
|
|
||
| assert json_response(conn, 200)["results"] == [ | ||
| %{"name" => "Google", "visitors" => 2}, | ||
| %{"name" => "DuckDuckGo", "visitors" => 1} | ||
| ] | ||
| end |
There was a problem hiding this comment.
Just an additional sanity check for already existing functionality :)
apata
force-pushed
the
list-sort
branch
2 times, most recently
from
d609428
to
68ebb7a
Compare
| @@ -164,6 +165,11 @@ defmodule Plausible.Stats.Legacy.QueryBuilder do | |||
| end | |||
| end | |||
|
|
|||
| defp put_order_by(query, %{} = params) do | |||
| order_by = Map.get(params, "order_by", nil) |> Filters.parse_order_by() | |||
There was a problem hiding this comment.
It isn't clear Filters.parse_order_by is part of 'legacy' parsing code that we can delete soon and shouldn't be used outside of it.
Instead of calling Filters module we should either:
a) Make Plausible.Stats.Filters.QueryParser.parse_order_by public
or
b) Make it a defp in this class instead of in the filters module
There was a problem hiding this comment.
nil is also default when no value is found, so Map.get(params, "order_by") suffices, as well as params["order_by"]
There was a problem hiding this comment.
Thanks for the suggestions! Went with B, but got a warning on doc string (and therefore my doctests) being ignored on private functions, had to keep it def.
apata
force-pushed
the
list-sort
branch
2 times, most recently
from
e93f9a8
to
8581b1f
Compare
|
👋 @apata The new "order_by [[visit_duration, asc], [visit:source, desc]]] is respected and flipping the sort orders works" test fails CI in #4491: https://github.com/plausible/analytics/actions/runs/10785402818/job/29910533968 |
| order_by_asc = Jason.encode!([["visit_duration", "asc"], ["visit:source", "desc"]]) | ||
|
|
||
| conn1 = | ||
| get(conn, "/api/stats/#{site.domain}/sources?detailed=true&order_by=#{order_by_asc}") |
There was a problem hiding this comment.
Is there any time filtering happening here? If so, should the timestamps in pageviews be dynamic?
There was a problem hiding this comment.
Seems like it filters the last month by default, since it started failing today.
Changes
Tests
Changelog
Documentation
Dark mode