Two fixes related to main graph and revenue metrics#6331
Conversation
This fixes a 500 error where we're trying to optimistically fetch the main graph with a stored revenue metric, while the goal filter has been removed. Rather than dropping revenue metrics in QueryOptimizer (which does not validate anything), we will do it in QueryBuilder instead. Query building will fail when dropping revenue metrics leaves query.metrics empty.
RobertJoonas
force-pushed
the
fix-graph-revenue-metric-500
branch
from
e9a5f22 to
2df67ea
Compare
| {%{relative_date: relative_date}, query_fields} = | ||
| Map.split(parsed_query_params, [:relative_date, :skip_goal_existence_check]) | ||
|
|
||
| struct!(%Query{}, Map.to_list(query_fields)) |
There was a problem hiding this comment.
much nicer like this, you could start the pipeline with %Query{} too. There's also no need for Map.to_list/1, the signature is struct!(module() | struct(), Enumerable.t()) :: struct()
There was a problem hiding this comment.
If the intention is to rewrite ParsedQueryParams into Query struct, whenever a key exists on both ends we could do something like:
Map.take(parsed_query_params, Map.keys(Query.__struct__())
and then refer to parsed_query_params.relative_date
There was a problem hiding this comment.
And perhaps move this to ParsedQueryParams.to_query()?
There was a problem hiding this comment.
We're Map.splitting a struct so query_fields is a struct too. Removing Map.to_list results in (Protocol.UndefinedError) protocol Enumerable not implemented for Plausible.Stats.ParsedQueryParams (a struct).
There was a problem hiding this comment.
For some reason didn't see the other 2 comments before. But your suggested approach sounds good. I've extracted ParsedQueryParams.to_query!/1 that finds the common query fields and builds the %Query{} struct.
|
|
||
| defp do_build(parsed_query_params, site, debug_metadata) do | ||
| now = parsed_query_params.now || DateTime.utc_now(:second) | ||
| {%{relative_date: relative_date}, query_fields} = |
There was a problem hiding this comment.
Does it effectively mean we discard skip_goal_existence_check? Unclear what's the reasoning behind it. Not sure why do we split at all, if we only care about relative_date. Can you elaborate?
There was a problem hiding this comment.
We split to get a map with direct query fields. Not everything in ParsedQueryParams will make it into the actual query struct. Some fields are just required to build a query.
relative_date is used for constructing a utc_time_range which is why this function needs it. As for skip_goal_existence_check that's used elsewhere in the QueryBuilder.build pipeline. This function simply ignores it indeed.
| if query.include.drop_unavailable_revenue_metrics and | ||
| map_size(query.revenue_currencies) == 0 do |
There was a problem hiding this comment.
maybe pattern match against that in function head/guard?
| skip_goal_existence_check: false | ||
|
|
||
| def to_query!(%__MODULE__{} = parsed_query_params) do | ||
| query_fields = (Query.__struct__() |> Map.keys()) -- [:__struct__] |
There was a problem hiding this comment.
%Query{} |> Map.from_struct() |> Map.keys()
2 participants
Changes
QueryBuilder.do_builda bit better, using a pipeline with steps rather than a long function.Tests
Changelog
Documentation
Dark mode