Skip to content

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Dec 4, 2025

User description

Benchmark PR plausible#5908

Type: Clean (correct implementation)

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

  • Clean up test cases related to flag behavior

  • Remove flag initialization from seeds and test setup


Diagram Walkthrough

flowchart LR
  A["Feature Flag Checks"] -->|Remove| B["Direct Feature Logic"]
  C["Test Cases"] -->|Remove Flag Tests| D["Simplified Tests"]
  E["Initialization Code"] -->|Remove| F["Clean Setup Files"]
  B --> G["Consolidated View Module"]
  D --> G
  F --> G
Loading

File Walkthrough

Relevant files
Enhancement
consolidated_view.ex
Remove feature flag checks 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 logic
  • Remove flag check from enable/1 function error handling
  • Simplify conditional flow by eliminating feature flag dependency
+0/-9     
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 by removing nested if
    statement
  • Remove :unavailable reason assignment when flag was disabled
  • Flatten control flow to directly call ConsolidatedView.enable/1
+15/-23 
Configuration changes
seeds.exs
Remove feature flag enablement from seeds                               

priv/repo/seeds.exs

  • Remove FunWithFlags.enable(:consolidated_view) call
  • Clean up seed initialization code
+0/-2     
test_helper.exs
Remove feature flag enablement from test setup                     

test/test_helper.exs

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

test/plausible/consolidated_view_test.exs

  • Remove test case for flag disabled scenario
  • Keep test for consolidated view disabled state
+0/-5     
sites_test.exs
Remove feature flag-specific test cases                                   

test/plausible_web/live/sites_test.exs

  • Remove two test cases checking behavior when flag is disabled during
    trial
  • Remove test for flag disabled state after trial ends
  • Keep existing tests for other consolidated view scenarios
+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: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

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: The new logic enabling or checking consolidated view availability performs critical state
checks and transitions without any added auditing or logging to trace who attempted or
changed view state and why.

Referred Code
@spec ok_to_display?(Team.t() | nil) :: boolean()
def ok_to_display?(team) do
  is_struct(team, Team) and
    view_enabled?(team) and
    has_sites_to_consolidate?(team) and
    Plausible.Billing.Feature.ConsolidatedView.check_availability(team) == :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
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Avoid redundant function calls

To avoid redundancy, call ConsolidatedView.can_manage? and
ConsolidatedView.cta_dismissed? once before the case statement, store their
results in variables, and reuse them in both branches.

lib/plausible_web/live/sites.ex [1093-1111]

 defp init_consolidated_view_assigns(user, team) do
+  can_manage? = ConsolidatedView.can_manage?(user, team)
+  cta_dismissed? = ConsolidatedView.cta_dismissed?(user, team)
+
   case ConsolidatedView.enable(team) do
     {:ok, view} ->
       %{
         consolidated_view: view,
-        can_manage_consolidated_view?: ConsolidatedView.can_manage?(user, team),
+        can_manage_consolidated_view?: can_manage?,
         consolidated_stats: :loading,
         no_consolidated_view_reason: nil,
-        consolidated_view_cta_dismissed?: ConsolidatedView.cta_dismissed?(user, team)
+        consolidated_view_cta_dismissed?: cta_dismissed?
       }
 
     {: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)
+        can_manage_consolidated_view?: can_manage?,
+        consolidated_view_cta_dismissed?: cta_dismissed?
       )
   end
 end
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies redundant function calls and proposes a valid refactoring that improves code quality by adhering to the DRY principle, making the code cleaner and slightly more performant.

Low
  • 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.

2 participants