Skip to content

Conversation

@RobertJoonas
Copy link
Contributor

Changes

When email reports (such as weekly reports, or traffic drop notifications) have been scheduled for consolidated views, we need to check upon sending, whether the consolidated view is still "eligible".

Tests

  • Automated tests have been added

Changelog

  • This PR does not make a user-facing change

Documentation

  • This change does not need a documentation update

Dark mode

  • This PR does not change the UI

@RobertJoonas RobertJoonas requested a review from aerosol November 12, 2025 14:47
Comment on lines 233 to 234
not Plausible.Sites.consolidated?(site) or
Plausible.ConsolidatedView.ok_to_display?(site.team)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe something verbose but expressive like:

Suggested change
not Plausible.Sites.consolidated?(site) or
Plausible.ConsolidatedView.ok_to_display?(site.team)
Plausible.Sites.regular?(site) or
(Plausible.Sites.consolidated?(site) and Plausible.ConsolidatedView.ok_to_display?(site.team))

Copy link
Contributor Author

Choose a reason for hiding this comment

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


:ok
defp handle_notification(%TrafficChangeNotification{type: :drop} = notification, now) do
current_visitors = Clickhouse.current_visitors_12h(notification.site)
Copy link
Member

Choose a reason for hiding this comment

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

ooh, does this already work with multiple site ids?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. #5876 made it work.


if report do
with %Plausible.Site{} <- site,
%{} = report <- Map.get(site, report_type),
Copy link
Member

Choose a reason for hiding this comment

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

fetch!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, makes sense. 2b9223f

@RobertJoonas RobertJoonas added this pull request to the merge queue Nov 13, 2025
Merged via the queue into master with commit 024e6bb Nov 13, 2025
16 checks passed
@RobertJoonas RobertJoonas deleted the no-emails-for-hidden-consolidated-views branch November 13, 2025 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants