Skip to content

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Dec 4, 2025

User description

Benchmark PR plausible#5908

Type: Corrupted (contains bugs)

Original PR Title: Remove :consolidated_view feature flag
Original PR Description: unknown
Original PR URL: plausible#5908


PR Type

Enhancement


Description

  • Remove :consolidated_view feature flag from codebase

  • Simplify conditional logic by eliminating flag checks

  • Remove feature flag initialization from seeds and tests

  • Update consolidated view enable logic to remove flag dependency


Diagram Walkthrough

flowchart LR
  A["Feature Flag Checks"] -->|Remove| B["Direct Enable Logic"]
  C["Flag Initialization"] -->|Remove| D["Simplified Setup"]
  B --> E["Consolidated View Module"]
  D --> F["Test & Seed Files"]
Loading

File Walkthrough

Relevant files
Enhancement
consolidated_view.ex
Remove feature flag dependency from consolidated view       

extra/lib/plausible/consolidated_view.ex

  • Remove flag_enabled?/1 function that checked FunWithFlags status
  • Remove flag check from ok_to_display?/1 condition
  • Simplify enable/1 function by removing flag and availability check
    conditions
  • Reorder remaining conditions for clarity
+1/-15   
sites.ex
Simplify consolidated view initialization logic                   

lib/plausible_web/live/sites.ex

  • Remove conditional check for ConsolidatedView.flag_enabled?/1
  • Simplify init_consolidated_view_assigns/2 to directly call
    ConsolidatedView.enable/1
  • Change consolidated_view_cta_dismissed? to always return false on
    success
  • Flatten nested if-else structure
+15/-23 
Configuration changes
seeds.exs
Remove feature flag initialization from seeds                       

priv/repo/seeds.exs

  • Remove FunWithFlags.enable(:consolidated_view) call from seed file
+0/-2     
test_helper.exs
Remove feature flag initialization from tests                       

test/test_helper.exs

  • Remove FunWithFlags.enable(:consolidated_view) call from test helper
    setup
+0/-2     
Tests
consolidated_view_test.exs
Remove feature flag test case                                                       

test/plausible/consolidated_view_test.exs

  • Remove test case for disabled feature flag scenario
  • Keep remaining tests for consolidated view functionality
+0/-5     
sites_test.exs
Remove feature flag-dependent test cases                                 

test/plausible_web/live/sites_test.exs

  • Remove two test cases that verify consolidated view behavior when flag
    is disabled
  • Tests covered scenarios during trial and after trial ends with flag
    down
+0/-46   

@qodo-code-review
Copy link

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 Log: The new flow enabling consolidated view and changing UI state does not emit any audit/log
entries for critical actions, making it unclear if such actions are traceable.

Referred Code
case ConsolidatedView.enable(team) do
  {:ok, view} ->
    %{
      consolidated_view: view,
      can_manage_consolidated_view?: ConsolidatedView.can_manage?(user, team),
      consolidated_stats: :loading,
      no_consolidated_view_reason: nil,
      consolidated_view_cta_dismissed?: false
    }

  {:error, reason} ->
    no_consolidated_view(
      no_consolidated_view_reason: reason,
      can_manage_consolidated_view?: ConsolidatedView.can_manage?(user, team),
      consolidated_view_cta_dismissed?: ConsolidatedView.cta_dismissed?(user, 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:
Reduced Safeguards: The updated enable/ok_to_display? logic removed availability checks and now relies on
fewer guards, which may allow attempts to enable when the feature is unavailable without
explicit error context.

Referred Code
def ok_to_display?(team) do
  is_struct(team, Team) and
    has_sites_to_consolidate?(team) and
    view_enabled?(team) and
    Plausible.Billing.Feature.ConsolidatedView.check_availability(team) == :ok
end

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
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Restore missing billing availability check

Restore the billing availability check in the enable/1 function. Removing this
check could allow users to enable a feature for which they are not on a valid
plan.

extra/lib/plausible/consolidated_view.ex [82-92]

 def enable(%Team{} = team) do
+  availability_check = Plausible.Billing.Feature.ConsolidatedView.check_availability(team)
+
   cond do
     not has_sites_to_consolidate?(team) ->
       {:error, :no_sites}
+
+    availability_check != :ok ->
+      availability_check
 
     not Teams.setup?(team) ->
       {:error, :team_not_setup}
 
     true ->
       do_enable(team)
   end
 end

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies that removing the billing availability check from the enable/1 function could allow users to bypass billing restrictions, which is a critical issue.

High
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants