Conversation
RobertJoonas
force-pushed
the
consolidated-site-settings
branch
from
7c48582 to
1004043
Compare
RobertJoonas
changed the title
aerosol
left a comment
aerosol
left a comment
There was a problem hiding this comment.
Love it, @RobertJoonas, great work. Some suggestions inline. Once we follow up with route-level authentication enforcement this should be complete.
lib/plausible/goals/goals.ex
Outdated
| end | ||
|
|
||
| defp check_no_currency_if_consolidated(site, changeset) do | ||
| if not is_nil(Ecto.Changeset.get_field(changeset, :currency)) and |
There was a problem hiding this comment.
The not is_nil seems to be unnecessary? Also flip the order of those predicates, so that we circuit break sooner.
lib/plausible/props.ex
Outdated
| on_ee do | ||
| defp site_id_filter(%Plausible.Site{} = site) do | ||
| if Plausible.Sites.consolidated?(site) do | ||
| site_ids = Plausible.ConsolidatedView.Cache.get(site.team.identifier) |
There was a problem hiding this comment.
Wondering if site.domain, while technically the same, would be more appropriate at this level. I'm thinking we could also change other call sites by the way. The reasoning being, we have access to the view-site itself, and the cache is built on per-site basis:
select: %{
consolidated_view_id: sc.domain,
Another thing is, are we going to lose the db connection if the query is sent over the wire with @max_sites_per_view? If so, this demands some clever approach or a compromise.
Try something like:
for i <- 1..15000, do: new_site(owner: u, domain: "crash-#{i}.example.com")
And refresh/restart the cache to see what happens.
There was a problem hiding this comment.
Make sense to lookup site_ids by domain instead of site.team.identifier. c675ec4
Another thing is, are we going to lose the db connection if the query is sent over the wire with @max_sites_per_view? If so, this demands some clever approach or a compromise.
Didn't quite catch what you're saying here? But I did try with 15k sites and it worked well. Even confirmed that if I send a pageview with props: %{author: "john"} to crash10000.com then it shows up nicely in the prop suggestions for the consolidated view.
There was a problem hiding this comment.
Yeah I meant to check if the "props query" won't be too big with a huge list of site ids. But I understand you're confirming it's fine.
RobertJoonas
changed the title
3 participants
Changes
Tests
Changelog
Documentation
Dark mode