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

[tomerqodo]

User description

Benchmark PR plausible#5760

Type: Corrupted (contains bugs)

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, Bug fix

---

Description

• Implement consolidated view lifecycle management with automatic enable/disable

• Add reset_if_enabled/1 to handle view state on site removal and transfer

• Ensure consistent site ID ordering in consolidated view cache queries

• Always update stats start dates for consolidated views on access

• Introduce enabled?/1 helper and refactor stats date handling

---

Diagram Walkthrough

flowchart LR
  A["Site Removal"] -->|"regular site"| B["reset_if_enabled"]
  C["Site Transfer"] -->|"between teams"| B
  B -->|"has sites"| D["Update stats dates"]
  B -->|"no sites"| E["Disable view"]
  F["Stats Access"] -->|"consolidated view"| G["Refresh dates"]
  H["View Enable"] -->|"create"| I["Set initial dates"]

Loading

File Walkthrough

	Relevant files
Enhancement	4 files consolidated_view.ex Consolidated view lifecycle and stats date management        +62/-19  removal.ex Disable consolidated view on regular site removal                +4/-0      sites.ex Always update consolidated view stats dates on access        +10/-15  invitations.ex Reset consolidated view on site transfer between teams      +1/-0
Bug fix	1 files cache.ex Add ORDER BY clause for consistent site ID ordering            +5/-2
Tests	4 files consolidated_view_test.exs Add tests for reset_if_enabled and stats date initialization +38/-15  site_removal_test.exs Test consolidated view disable on site removal scenarios  +41/-10  sites_test.exs Test consolidated view stats date refresh behavior              +19/-0    transfer_test.exs Test consolidated view disable on site 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 Auditing: Newly introduced critical lifecycle actions (enable/disable/reset consolidated views and stats date updates) do not emit audit logs with actor, timestamp, action, and outcome, which may hinder event reconstruction. Referred Code @spec reset_if_enabled(Team.t()) :: :ok def reset_if_enabled(%Team{} = team) do case get(team) do nil -> :skip consolidated_view -> if has_sites_to_consolidate?(consolidated_view.team) do consolidated_view |> change_stats_dates(team) |> bump_updated_at() |> Repo.update!() else disable(team) end end :ok 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: Functions like reset_if_enabled/1 and change paths use Repo.update!/Repo.insert! which raise on failure without contextual handling, risking ungraceful failures and missing diagnostic context. 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	Site transfer logic is incomplete The site transfer logic is missing a step. It should update the consolidated view for the source team, not just the destination team, by calling Plausible.ConsolidatedView.reset_if_enabled(prior_team). Examples: lib/plausible/teams/invitations.ex [440-444] on_ee do Billing.SiteLocker.update_for(prior_team, send_email?: false) :unlocked = Billing.SiteLocker.update_for(team, send_email?: false) Plausible.ConsolidatedView.reset_if_enabled(team) end Solution Walkthrough: Before: def change_team(site, user, team): # ... site transfer logic ... prior_team = site.team # ... update site's team_id ... on_ee do Billing.SiteLocker.update_for(prior_team) Billing.SiteLocker.update_for(team) # Only the destination team's consolidated view is reset Plausible.ConsolidatedView.reset_if_enabled(team) end :ok After: def change_team(site, user, team): # ... site transfer logic ... prior_team = site.team # ... update site's team_id ... on_ee do Billing.SiteLocker.update_for(prior_team) Billing.SiteLocker.update_for(team) # Reset consolidated view for both source and destination teams Plausible.ConsolidatedView.reset_if_enabled(prior_team) Plausible.ConsolidatedView.reset_if_enabled(team) end :ok Suggestion importance[1-10]: 9 __ Why: This suggestion correctly identifies a significant bug where the source team's consolidated view is not updated after a site transfer, which contradicts the PR's stated goals and can lead to incorrect system state.	High
General	Avoid unnecessary database updates Refactor stats_start_date/1 to prevent unnecessary database updates by checking for actual changes before calling Repo.update!. 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 - |> Plausible.ConsolidatedView.change_stats_dates(team) - |> Repo.update!() - |> Map.fetch!(:stats_start_date) + changeset = Plausible.ConsolidatedView.change_stats_dates(site, team) + + updated_site = + if changeset.valid? and changeset.changes != %{} do + Repo.update!(changeset) + else + site + end + + Map.fetch!(updated_site, :stats_start_date) end end Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that a function named like a getter has a side effect of updating the database, and proposes a valid optimization to avoid unnecessary writes.	Medium
More