Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor: Exclude selected prop keys in API instead of in FE #3782

Closed
wants to merge 1 commit into from
Closed

Refactor: Exclude selected prop keys in API instead of in FE #3782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 1 addition & 3 deletions assets/js/dashboard/components/combobox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,7 @@ export default function PlausibleCombobox(props) {
}

function isOptionDisabled(option) {
const optionAlreadySelected = props.values.some((val) => val.value === option.value)
const optionDisabled = (props.disabledOptions || []).some((val) => val?.value === option.value)
return optionAlreadySelected || optionDisabled
return props.values.some((val) => val.value === option.value)
}

function fetchOptions(query) {
Expand Down
2 changes: 1 addition & 1 deletion assets/js/dashboard/stats/modals/prop-filter-modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ function PropFilterModal(props) {
const query = parseQuery(props.location.search, props.site)
const [formState, setFormState] = useState(getFormState(query))

const selectedPropKeys = useMemo(() => Object.values(formState.values).map((value) => value.propKey), [formState])
const selectedPropKeys = useMemo(() => Object.values(formState.values).map((value) => value.propKey).filter((value) => value), [formState])

function onPropKeySelect(id, selectedOptions) {
const newPropKey = selectedOptions.length === 0 ? null : selectedOptions[0]
Expand Down
4 changes: 2 additions & 2 deletions assets/js/dashboard/stats/modals/prop-filter-row.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ function PropFilterRow({
}) {
function fetchPropKeyOptions() {
return (input) => {
return api.get(apiPath(site, "/suggestions/prop_key"), query, { q: input.trim() })
const exclude = selectedPropKeys.map((key) => key.value)
return api.get(apiPath(site, "/suggestions/prop_key"), query, { q: input.trim(), exclude: JSON.stringify(exclude) })
Copy link
Contributor

Choose a reason for hiding this comment

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

I think instead of specifying an additional exclude parameter, we should use the formState to update the query object sent to the backend instead. That is similar to all other filter modals.

Right now, if you do the following:

  1. setup: send two events on localhost:
  • mix send_pageview --props "{\"one\":\"1\"}"
  • mix send_pageview --props "{\"two\":\"2\"}"
  • configure these props in Site Settings > Custom Properties
  1. Open the prop filter modal and apply the filter "Property one is 1
  2. Click on the applied filter label on top of the dashboard to open the filter modal again
  3. Click "+ Add another"
  4. Remove the already applied one clicking the red trash icon
  5. Try to select a prop_key

After step 5, two will not be returned anymore, because the suggestions request will be unaware of what is happening in the formState.

Now there's a small blocker here I believe - how do you update the query with the current formState if a prop_key is selected, but the prop_value corresponding to it is empty?

Perhaps we could solve this by disabling (or hiding) the "+ Add another" button when any of the rows is incomplete? Then, the situation where a prop_key is hanging in the formstate without a prop_value would become impossible

}
}

Expand All @@ -50,7 +51,6 @@ function PropFilterRow({
values={propKey ? [propKey] : []}
onSelect={(value) => onPropKeySelect(id, value)}
placeholder={'Property'}
disabledOptions={selectedPropKeys}
/>
</div>
<div className="col-span-3 mx-2">
Expand Down
4 changes: 2 additions & 2 deletions lib/plausible/stats.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ defmodule Plausible.Stats do
end
end

def filter_suggestions(site, query, filter_name, filter_search) do
def filter_suggestions(site, query, filter_name, filter_search, exclude \\ %{}) do
include_sentry_replay_info()
FilterSuggestions.filter_suggestions(site, query, filter_name, filter_search)
FilterSuggestions.filter_suggestions(site, query, filter_name, filter_search, exclude)
end
end
19 changes: 10 additions & 9 deletions lib/plausible/stats/filter_suggestions.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ defmodule Plausible.Stats.FilterSuggestions do
import Plausible.Stats.Base
alias Plausible.Stats.Query

def filter_suggestions(site, query, "country", filter_search) do
def filter_suggestions(site, query, "country", filter_search, _exclude) do
matches = Location.search_country(filter_search)

q =
Expand All @@ -28,7 +28,7 @@ defmodule Plausible.Stats.FilterSuggestions do
end)
end

def filter_suggestions(site, query, "region", "") do
def filter_suggestions(site, query, "region", "", _exclude) do
from(
e in query_sessions(site, query),
group_by: e.subdivision1_code,
Expand All @@ -48,7 +48,7 @@ defmodule Plausible.Stats.FilterSuggestions do
end)
end

def filter_suggestions(site, query, "region", filter_search) do
def filter_suggestions(site, query, "region", filter_search, _exclude) do
matches = Location.search_subdivision(filter_search)

q =
Expand All @@ -71,7 +71,7 @@ defmodule Plausible.Stats.FilterSuggestions do
end)
end

def filter_suggestions(site, query, "city", "") do
def filter_suggestions(site, query, "city", "", _exclude) do
from(
e in query_sessions(site, query),
group_by: e.city_geoname_id,
Expand All @@ -91,7 +91,7 @@ defmodule Plausible.Stats.FilterSuggestions do
end)
end

def filter_suggestions(site, query, "city", filter_search) do
def filter_suggestions(site, query, "city", filter_search, _exclude) do
filter_search = String.downcase(filter_search)

q =
Expand All @@ -118,7 +118,7 @@ defmodule Plausible.Stats.FilterSuggestions do
end)
end

def filter_suggestions(site, _query, "goal", filter_search) do
def filter_suggestions(site, _query, "goal", filter_search, _exclude) do
site
|> Plausible.Goals.for_site()
|> Enum.map(fn x -> if x.event_name, do: x.event_name, else: "Visit #{x.page_path}" end)
Expand All @@ -131,14 +131,15 @@ defmodule Plausible.Stats.FilterSuggestions do
|> wrap_suggestions()
end

def filter_suggestions(site, query, "prop_key", filter_search) do
def filter_suggestions(site, query, "prop_key", filter_search, exclude) do
filter_query = if filter_search == nil, do: "%", else: "%#{filter_search}%"

from(e in base_event_query(site, query),
array_join: meta in "meta",
as: :meta,
select: meta.key,
where: fragment("? ilike ?", meta.key, ^filter_query),
where: meta.key not in ^exclude,
group_by: meta.key,
order_by: [desc: fragment("count(*)")],
limit: 25
Expand All @@ -148,7 +149,7 @@ defmodule Plausible.Stats.FilterSuggestions do
|> wrap_suggestions()
end

def filter_suggestions(site, query, "prop_value", filter_search) do
def filter_suggestions(site, query, "prop_value", filter_search, _exclude) do
filter_query = if filter_search == nil, do: "%", else: "%#{filter_search}%"

{"event:props:" <> key, _filter} = Query.get_filter_by_prefix(query, "event:props")
Expand Down Expand Up @@ -180,7 +181,7 @@ defmodule Plausible.Stats.FilterSuggestions do
|> wrap_suggestions()
end

def filter_suggestions(site, query, filter_name, filter_search) do
def filter_suggestions(site, query, filter_name, filter_search, _exclude) do
filter_search = if filter_search == nil, do: "", else: filter_search

filter_query =
Expand Down
3 changes: 2 additions & 1 deletion lib/plausible_web/controllers/api/stats_controller.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1273,10 +1273,11 @@ defmodule PlausibleWeb.Api.StatsController do
site = conn.assigns[:site]

query = Query.from(site, params)
exclude = Jason.decode!(params["exclude"] || "[]")

json(
conn,
Stats.filter_suggestions(site, query, params["filter_name"], params["q"])
Stats.filter_suggestions(site, query, params["filter_name"], params["q"], exclude)
)
end

Expand Down
45 changes: 45 additions & 0 deletions test/plausible_web/controllers/api/stats_controller/suggestions_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,51 @@ defmodule PlausibleWeb.Api.StatsController.SuggestionsTest do
]
end

test "allowing excluding keys", %{conn: conn, site: site} do
populate_stats(site, [
build(:pageview,
"meta.key": ["author"],
"meta.value": ["Uku Taht"],
timestamp: ~N[2022-01-01 00:00:00]
),
build(:pageview,
"meta.key": ["author"],
"meta.value": ["Uku Taht"],
timestamp: ~N[2022-01-01 00:00:00]
),
build(:pageview,
"meta.key": ["author"],
"meta.value": ["Uku Taht"],
timestamp: ~N[2022-01-01 00:00:00]
),
build(:pageview,
"meta.key": ["logged_in"],
"meta.value": ["false"],
timestamp: ~N[2022-01-01 00:00:00]
),
build(:pageview,
"meta.key": ["logged_in"],
"meta.value": ["false"],
timestamp: ~N[2022-01-01 00:00:00]
),
build(:pageview,
"meta.key": ["dark_mode"],
"meta.value": ["true"],
timestamp: ~N[2022-01-01 00:00:00]
)
])

conn =
get(
conn,
"/api/stats/#{site.domain}/suggestions/prop_key?period=day&date=2022-01-01&exclude=[\"author\",\"logged_in\"]"
)

assert json_response(conn, 200) == [
%{"label" => "dark_mode", "value" => "dark_mode"}
]
end

test "returns suggestions found in time frame", %{
conn: conn,
site: site
Expand Down