[CLEAN] Synthetic Benchmark PR #5760 - Consolidated view lifecycle

[tomerqodo]

User description

Benchmark PR plausible#5760

Type: Clean (correct implementation)

Original PR Title: Consolidated view lifecycle
Original PR Description: ### Changes

With this change, some specifics to consolidated views are now handled:

• regular site removal disables consolidated view, if there's no sites to consolidate any more
• site transfer disables consolidated view in the prior team, if there's no sites to consolidate any more
• regular site IDs ordering is now uniform, regardless if cached
• stats_start_date and native_stats_start_at are always queried and updated, if need be, whether the view has been just enabled or accessed

Tests

• Automated tests have been added
• This PR does not require tests

Changelog

• Entry has been added to changelog
• This PR does not make a user-facing change

Documentation

• Docs have been updated
• This change does not need a documentation update

Dark mode

• The UI has been tested both in dark and light mode
• This PR does not change the UI

Original PR URL: plausible#5760

---

PR Type

Enhancement

---

Description

• Add reset_if_enabled/1 function to handle consolidated view lifecycle

• Implement enabled?/1 predicate and refactor stats date management

• Ensure site IDs ordering is uniform with explicit ORDER BY clause

• Integrate consolidated view reset on site removal and team transfers

• Add comprehensive tests for consolidated view state transitions

---

Diagram Walkthrough

flowchart LR
  A["Site Removal"] -->|triggers| B["reset_if_enabled"]
  C["Team Transfer"] -->|triggers| B
  B -->|checks| D["has_sites_to_consolidate?"]
  D -->|yes| E["Update stats dates"]
  D -->|no| F["Disable view"]
  E --> G["Consolidated View"]
  F --> G
  H["stats_start_date query"] -->|always updates| G

Loading

File Walkthrough

	Relevant files
Enhancement	4 files consolidated_view.ex Add lifecycle management and refactor stats handling          +62/-19  removal.ex Disable consolidated view on regular site removal                +4/-0      sites.ex Always update consolidated view stats dates                            +10/-15  invitations.ex Reset consolidated view on site team transfer                        +1/-0
Bug fix	1 files cache.ex Ensure uniform site IDs ordering in queries                            +5/-2
Tests	4 files consolidated_view_test.exs Add tests for reset and enabled state checks                          +38/-15  site_removal_test.exs Test consolidated view disabling on site removal                  +41/-10  sites_test.exs Test consolidated view stats date updates                                +19/-0    transfer_test.exs Test consolidated view disabling on transfer                          +24/-0

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing audit logs: Newly added critical actions like consolidated view reset on site removal and team transfer are not accompanied by explicit audit logging, which may hinder reconstructing events unless handled elsewhere. Referred Code if Plausible.Sites.regular?(site) do :ok = Plausible.ConsolidatedView.reset_if_enabled(site.team) end Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Bang update: The new flow uses Repo.update! which will raise on failure without local handling, potentially causing crashes if database updates fail unless upstream supervision is intended. Referred Code consolidated_view |> change_stats_dates(team) |> bump_updated_at() |> Repo.update!() else disable(team) end end :ok Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Avoid database writes in getter functions The Sites.stats_start_date/1 function for consolidated views should not perform a database write. This update logic should instead be triggered by events like site creation, similar to how it's handled for site removal and transfers. Examples: lib/plausible/sites.ex [353-360] def stats_start_date(%Site{consolidated: true} = site) do team = Repo.preload(site, :team).team site |> Plausible.ConsolidatedView.change_stats_dates(team) |> Repo.update!() |> Map.fetch!(:stats_start_date) end lib/plausible/site/removal.ex [23-25] if Plausible.Sites.regular?(site) do :ok = Plausible.ConsolidatedView.reset_if_enabled(site.team) end Solution Walkthrough: Before: # In lib/plausible/sites.ex def stats_start_date(%Site{consolidated: true} = site) do # This function is expected to be a read-only "getter" team = Repo.preload(site, :team).team site |> Plausible.ConsolidatedView.change_stats_dates(team) # Queries DB |> Repo.update!() # <-- Writes to DB on every call |> Map.fetch!(:stats_start_date) end After: # In lib/plausible/sites.ex # The write logic is removed from the getter. def stats_start_date(%Site{consolidated: true, stats_start_date: date}) do date end # In lib/plausible/sites.ex (or similar module for site creation) def create_site(attrs) do # ... site creation logic ... {:ok, site} = Repo.insert(changeset) # Trigger update explicitly after creation Plausible.ConsolidatedView.reset_if_enabled(site.team) {:ok, site} end Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a significant design flaw in Sites.stats_start_date/1 that violates the Command-Query Separation principle by performing a database write within a getter, which can lead to performance and predictability issues.	High
Possible issue	Prevent potential runtime error on preload To prevent a potential KeyError, use Map.get/2 to safely access the preloaded :team association instead of using dot notation. lib/plausible/sites.ex [351-361] on_ee do # for now, we're going to always update consolidated views def stats_start_date(%Site{consolidated: true} = site) do - team = Repo.preload(site, :team).team + site_with_team = Repo.preload(site, :team) + team = Map.get(site_with_team, :team) site |> Plausible.ConsolidatedView.change_stats_dates(team) |> Repo.update!() |> Map.fetch!(:stats_start_date) end end Apply / Chat Suggestion importance[1-10]: 2 __ Why: The suggestion addresses a theoretical edge case where a consolidated site might not have a team, but this is unlikely given the application logic, and the proposed fix is incomplete as it would just cause a different error downstream.	Low
More