-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Consolidated View life cycle + billing integration #5866
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.
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
base: master
Are you sure you want to change the base?
Conversation
ea6644d to
b1e1b17
Compare
2fa36cd to
1b9decd
Compare
- add functions to manipulate user/team options (for CTA) - require at least two sites in order to create a consolidated view - require billing/plan compliance when computing eligibility
Co-authored-by: Sanne de Vries <[email protected]> Co-authored-by: Uku Taht <[email protected]>
- require team-wise feature flag instead of super admin role - redirect to /sites if the team isn't eligible any more - enforce regular site in shared links controller
1b9decd to
87e593f
Compare
64760be to
2ae4797
Compare
RobertJoonas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff, looks great overall! A few comments inline.
| def ok_to_display?(team, user) do | ||
| with %Team{} <- team, | ||
| %User{} <- user, | ||
| true <- Plausible.Auth.is_super_admin?(user), | ||
| true <- enabled?(team), | ||
| true <- has_sites_to_consolidate?(team) do | ||
| true <- flag_enabled?(team), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user argument here is unnecessary I believe. I would like to use this function when checking whether a stats report or traffic spike email should be sent for a consolidated view. There's no user in that context.
| phx-click="delete-consolidated-view" | ||
| phx-target={@myself} | ||
| data-confirm="Are you sure you want to delete this consolidated view?" | ||
| data-confirm="Are you sure you want to delete this consolidated view? It will be recreated whenever eligible subscription/trial accesses /sites for that team." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also mention that the configuration gets lost with this action?
| consolidated_view_available? = | ||
| on_ee( | ||
| do: Plausible.ConsolidatedView.ok_to_display?(shared_link.site.team, current_user), | ||
| else: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to just say that this is false for shared links too? Even if I'm logged in and seeing my own shared link, I will not be able to switch to the consolidated view in the site picker, or in fact, even see any of my other sites there. There's a login link at the top right though.
| <!-- The `z-49` class is to make the dropdown appear above the site cards and (TODO) below the top-right drop down. --> | ||
| <!-- The proper solution is for Prima to render the dropdown menu within a <.portal> element to avoid --> | ||
| <!-- any stacking context issues. TODO --> | ||
| <PrimaDropdown.dropdown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| end | ||
|
|
||
| def upgrade_card(assigns) do | ||
| def consolidated_view_card_cta(assigns) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a slight UI annoyance I'm seeing locally. Try to hide the CTA card, then navigate to another page, e.g. click on one of the site cards to see a dashboard. Now, hit back in the browser history. The card flickers for a second and then disappears. Same happens when you've hidden it before and then restore it.
We should make sure that hiding/restoring the card also updates the entry in browser history stack.
| assert stats =~ "Views per visit 1.33" | ||
| end | ||
|
|
||
| test "consolidated view does not show up for non-superadmin (temp)", %{conn: conn} do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer necessar I think? Looks like a duplicate of "single site won't show neither CTA or view - team setup"

Changes
This PR integrates consolidated view life cycle with billing properties.
It relies on two migrations which will be extracted to separate PRs once the review is concluded.
Main changes:
:consolidated_views- we no longer display anything based onsuper_adminrole; flag is not setup on production yet, meaning existing consolidated views will disappear (and no CTAs will be shown).ConsolidatedView/sitesis visited an attempt is made to create consolidated view. If the team is eligible, and the feature flag is raised, the consolidated view card is shown. Otherwise a CTA card is displayed (courtesy of @sanne-san)/siteswill include the consolidated view card."+ New consolidated view"dropdown item)/sitesConsolidatedView.enabled?/1has been removed, since enabling doesn't mean availabilityTODO:
record-2025-11-10-10-29-34-year.node.norm.mp4
Tests
Changelog
Documentation
Dark mode