Consolidated view email reports #5876
Conversation
RobertJoonas
force-pushed
the
consolidated-view-email-reports
branch
from
c70dfe4 to
b0cb685
Compare
RobertJoonas
force-pushed
the
consolidated-view-email-reports
branch
from
b0cb685 to
742fa67
Compare
| if consolidated?(site), do: "consolidated view", else: site.domain | ||
| def display_name(%Site{} = site, opts \\ []) do | ||
| if consolidated?(site) do | ||
| "#{if opts[:capitalize_consolidated], do: "C", else: "c"}onsolidated view" |
There was a problem hiding this comment.
perhaps use String.capitalize/1 exactly where it's needed 😅
There was a problem hiding this comment.
It's a bit annoying indeed, but on the other hand we also don't want to capitalize site.domain if it's not a consolidated site 😵💫
There was a problem hiding this comment.
Right, but maybe it is telling that the display_name concept is not applicable to cover all the cases.
So maybe we're completely fine with having an email template like:
<%= if Plausible.Sites.consolidated?(@site) do %>
Consolidated view
<% else %>
<%= @site.domain %>
<% end %>instead of
<%= Plausible.Sites.display_name(@site, capitalize_consolidated: true) %>The latter feels forced for no clear benefit; leaving it up to you though.
There was a problem hiding this comment.
Minor benefit perhaps is that it's easier to grep the display_name function and discover all places where the name "Consolidated view" is used instead of site.domain. Maybe feels a bit forced, but doesn't hurt readability IMO.
There was a problem hiding this comment.
Yes but there's plenty of "consolidated view" strings in there verbatim. And sometimes we call it "All sites" even. So it really doesn't serve any purpose.
There was a problem hiding this comment.
Plus there's like a gazillion of places where site.domain is used
There was a problem hiding this comment.
That said, your call still. Please excuse my OCD
lib/workers/send_email_report.ex
Outdated
| |> Repo.preload([report_type, :team]) | ||
| from(s in Plausible.Site, | ||
| where: s.id == ^site_id, | ||
| preload: [^report_type, :team] |
There was a problem hiding this comment.
This is still three queries 😛 You'd have to inner_join both associations.
iex(7)> from(s in Plausible.Site, where: s.id == 1, preload: [:monthly_report, :team]) |> Repo.one(); 1
08:35:56.443 [debug] QUERY OK source="sites" db=1.2ms idle=1072.4ms
SELECT s0."id", s0."domain", s0."timezone", s0."public", s0."stats_start_date", s0."native_stats_start_at", s0."allowed_event_props", s0."conversions_enabled", s0."props_enabled", s0."funnels_enabled", s0."legacy_time_on_page_cutoff", s0."consolidated", s0."ingest_rate_limit_scale_seconds", s0."ingest_rate_limit_threshold", s0."domain_changed_from", s0."domain_changed_at", s0."imported_data", s0."team_id", s0."inserted_at", s0."updated_at" FROM "sites" AS s0 WHERE (s0."id" = 1) []
08:35:56.444 [debug] QUERY OK source="teams" db=0.4ms idle=1074.5ms
SELECT t0."id", t0."identifier", t0."name", t0."trial_expiry_date", t0."accept_traffic_until", t0."allow_next_upgrade_override", t0."locked", t0."setup_complete", t0."setup_at", t0."hourly_api_request_limit", t0."notes", t0."policy", t0."grace_period", t0."inserted_at", t0."updated_at", t0."id" FROM "teams" AS t0 WHERE (t0."id" = $1) [1]
08:35:56.444 [debug] QUERY OK source="monthly_reports" db=0.4ms idle=1074.5ms
SELECT m0."id", m0."recipients", m0."site_id", m0."inserted_at", m0."updated_at", m0."site_id" FROM "monthly_reports" AS m0 WHERE (m0."site_id" = $1) [1]
Changes
Make the following email reports work for consolidated views:
Tests
Changelog
Documentation
Dark mode