Feature flag suggested changes

.env

@@ -29,3 +29,4 @@ SPLIT_PASSWORD=split_password
 SPLIT_USERNAME=split_username
 STRIPE_API_KEY=stripe_api_key
 STRIPE_PUBLISHABLE_KEY=stripe_publishable_key
+FEATURES_ADMIN_PASSWORD=supersecret

Gemfile

@@ -10,6 +10,9 @@ gem "autoprefixer-rails"
 gem "bourbon", "~> 5.0"
 gem "email_validator"
 gem "faraday"
+gem "flipper"
+gem "flipper-active_record"
+gem "flipper-ui"
 gem "haml-rails", "~> 2.0"
 gem "high_voltage"
 gem "inifile"

Gemfile.lock

@@ -122,6 +122,15 @@ GEM
     faraday (1.0.0)
       multipart-post (>= 1.2, < 3)
     ffi (1.12.2)
+    flipper (0.17.2)
+    flipper-active_record (0.17.2)
+      activerecord (>= 4.2, < 7)
+      flipper (~> 0.17.2)
+    flipper-ui (0.17.2)
+      erubi (>= 1.0.0, < 2.0.0)
+      flipper (~> 0.17.2)
+      rack (>= 1.4, < 3)
+      rack-protection (>= 1.5.3, < 2.1.0)
     foreman (0.87.0)
     globalid (0.4.2)
       activesupport (>= 4.2.0)
@@ -383,6 +392,9 @@ DEPENDENCIES
   email_validator
   factory_girl_rails
   faraday
+  flipper
+  flipper-active_record
+  flipper-ui
   foreman
   haml-rails (~> 2.0)
   high_voltage

app/models/owner.rb

@@ -64,6 +64,10 @@ def metered_plan?
     whitelisted?
   end
 
+  def flipper_id
+    "Owner:#{id}"
+  end
+
   private
 
   def hound_config

app/models/violation.rb

@@ -23,6 +23,10 @@ def source
     crypt.decrypt_and_verify(self[:source]) if self[:source]
   end
 
+  def owner
+    file_review.build.repo.owner
+  end
+
   private
 
   def crypt

app/services/suggest_changes.rb

@@ -7,17 +7,21 @@ def initialize(violation)
   end
 
   def call
-    violation.messages.each do |message|
-      apply_suggestion(message)
-      break if suggestion.present?
-    end
+    if Flipper[:suggested_changes].enabled?(violation.owner)
+      violation.messages.each do |message|
+        apply_suggestion(message)
+        break if suggestion.present?
+      end
 
-    messages = violation.messages.join(CommentingPolicy::COMMENT_LINE_DELIMITER)
+      formatted_messages = violation.messages.join(CommentingPolicy::COMMENT_LINE_DELIMITER)
 
-    if suggestion.blank?
-      messages
+      if suggestion.blank?
+        formatted_messages
+      else
+        formatted_messages + "<br>\n```suggestion\n#{suggestion}\n```"
+      end
     else
-      messages + "<br>\n```suggestion\n#{suggestion}\n```"
+      violation.messages.join(CommentingPolicy::COMMENT_LINE_DELIMITER)
     end
   end
 

config/initializers/flipper.rb

@@ -0,0 +1,8 @@
+require "flipper"
+require "flipper/adapters/active_record"
+
+Flipper.configure do |config|
+  config.default do
+    Flipper.new(Flipper::Adapters::ActiveRecord.new)
+  end
+end

config/routes.rb

@@ -20,6 +20,16 @@
   mount Sidekiq::Web, at: "/jobs"
   mount Split::Dashboard, at: "/split"
 
+  flipper_app = Flipper::UI.app(Flipper.instance) do |builder|
+    builder.use Rack::Auth::Basic do |_, password|
+      ActiveSupport::SecurityUtils.secure_compare(
+        ::Digest::SHA256.hexdigest(password),
+        ::Digest::SHA256.hexdigest(ENV["FEATURES_ADMIN_PASSWORD"]),
+      )
+    end
+  end
+  mount flipper_app, at: "/features"
+
   get "/auth/github/callback", to: "sessions#create"
   get "/sign_out", to: "sessions#destroy"
   get "/configuration", to: redirect(ENV.fetch("DOCS_URL"), status: 302)

db/migrate/20200401183732_create_flipper_tables.rb

@@ -0,0 +1,22 @@
+class CreateFlipperTables < ActiveRecord::Migration[6.0]
+  def self.up
+    create_table :flipper_features do |t|
+      t.string :key, null: false
+      t.timestamps null: false
+    end
+    add_index :flipper_features, :key, unique: true
+
+    create_table :flipper_gates do |t|
+      t.string :feature_key, null: false
+      t.string :key, null: false
+      t.string :value
+      t.timestamps null: false
+    end
+    add_index :flipper_gates, [:feature_key, :key, :value], unique: true
+  end
+
+  def self.down
+    drop_table :flipper_gates
+    drop_table :flipper_features
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2019_12_12_043041) do
+ActiveRecord::Schema.define(version: 2020_04_01_183732) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -62,7 +62,23 @@
     t.index ["build_id"], name: "index_file_reviews_on_build_id"
   end
 
-  create_table "memberships", id: :serial, force: :cascade do |t|
+  create_table "flipper_features", force: :cascade do |t|
+    t.string "key", null: false
+    t.datetime "created_at", precision: 6, null: false
+    t.datetime "updated_at", precision: 6, null: false
+    t.index ["key"], name: "index_flipper_features_on_key", unique: true
+  end
+
+  create_table "flipper_gates", force: :cascade do |t|
+    t.string "feature_key", null: false
+    t.string "key", null: false
+    t.string "value"
+    t.datetime "created_at", precision: 6, null: false
+    t.datetime "updated_at", precision: 6, null: false
+    t.index ["feature_key", "key", "value"], name: "index_flipper_gates_on_feature_key_and_key_and_value", unique: true
+  end
+
+  create_table "memberships", force: :cascade do |t|
     t.integer "user_id", null: false
     t.integer "repo_id", null: false
     t.datetime "created_at", null: false

spec/services/suggest_changes_spec.rb

@@ -4,89 +4,127 @@
 
 RSpec.describe SuggestChanges do
   describe ".call" do
-    context "single violation" do
-      it "makes a suggestion for a missing comma" do
-        violation = instance_double(
-          "Violation",
-          messages: [
-            "Put a comma after the last parameter of a multiline method call.",
-          ],
-          source: "    violation.fetch(:source)",
-        )
-        suggest_changes = described_class.new(violation)
-
-        result = suggest_changes.call
-
-        expect(result).to eq <<~COMMENT.chomp
-          Put a comma after the last parameter of a multiline method call.<br>
-          ```suggestion
-              violation.fetch(:source),
-          ```
-        COMMENT
+    context "when suggested changes are enabled" do
+      before do
+        flipper_feature = double(enabled?: true)
+        stub_const("Flipper", { suggested_changes: flipper_feature })
       end
 
-      it "makes suggestion for missing semicolon" do
-        violation = instance_double(
-          "Violation",
-          messages: [
-            "Missing semicolon semi",
-            "Something else",
-          ],
-          source: "  console.log('wat')",
-        )
-        suggest_changes = described_class.new(violation)
-
-        result = suggest_changes.call
-
-        expect(result).to eq <<~COMMENT.chomp
-          Missing semicolon semi<br>Something else<br>
-          ```suggestion
-            console.log('wat');
-          ```
-        COMMENT
+      context "single violation" do
+        it "makes a suggestion for a missing comma" do
+          violation = instance_double(
+            "Violation",
+            messages: [
+              "Put a comma after the last parameter of a multiline method call.",
+            ],
+            source: "    violation.fetch(:source)",
+            owner: double,
+          )
+          suggest_changes = described_class.new(violation)
+
+          result = suggest_changes.call
+
+          expect(result).to eq <<~COMMENT.chomp
+            Put a comma after the last parameter of a multiline method call.<br>
+            ```suggestion
+                violation.fetch(:source),
+            ```
+          COMMENT
+        end
+
+        it "makes suggestion for missing semicolon" do
+          violation = instance_double(
+            "Violation",
+            messages: [
+              "Missing semicolon semi",
+              "Something else",
+            ],
+            source: "  console.log('wat')",
+            owner: double,
+          )
+          suggest_changes = described_class.new(violation)
+
+          result = suggest_changes.call
+
+          expect(result).to eq <<~COMMENT.chomp
+            Missing semicolon semi<br>Something else<br>
+            ```suggestion
+              console.log('wat');
+            ```
+          COMMENT
+        end
+
+        it "makes suggestion for missing space after comma" do
+          violation = instance_double(
+            "Violation",
+            messages: [
+              "A space is required after ',' comma-spacing",
+            ],
+            source: "function wat(one,two, three) {",
+            owner: double,
+          )
+          suggest_changes = described_class.new(violation)
+
+          result = suggest_changes.call
+
+          expect(result).to eq <<~COMMENT.chomp
+            A space is required after ',' comma-spacing<br>
+            ```suggestion
+            function wat(one, two, three) {
+            ```
+          COMMENT
+        end
       end
 
-      it "makes suggestion for missing space after comma" do
-        violation = instance_double(
-          "Violation",
-          messages: [
-            "A space is required after ',' comma-spacing",
-          ],
-          source: "function wat(one,two, three) {",
-        )
-        suggest_changes = described_class.new(violation)
-
-        result = suggest_changes.call
-
-        expect(result).to eq <<~COMMENT.chomp
-          A space is required after ',' comma-spacing<br>
-          ```suggestion
-          function wat(one, two, three) {
-          ```
-        COMMENT
+      context "multiple violations" do
+        it "makes a suggestion for the first fixable violation" do
+          violation = instance_double(
+            "Violation",
+            messages: [
+              "Layout/TrailingWhitespace: Trailing whitespace detected.",
+              "Put a comma after the last parameter of a multiline method call.",
+            ],
+            source: "    violation.fetch(:source) ",
+            owner: double,
+          )
+          suggest_changes = described_class.new(violation)
+
+          result = suggest_changes.call
+
+          expect(result).to eq <<~COMMENT.chomp
+            Layout/TrailingWhitespace: Trailing whitespace detected.<br>Put a comma after the last parameter of a multiline method call.<br>
+            ```suggestion
+                violation.fetch(:source)
+            ```
+          COMMENT
+        end
       end
     end
 
-    context "multiple violations" do
-      it "makes a suggestion for the first fixable violation" do
-        violation = instance_double(
-          "Violation",
-          messages: [
-            "Layout/TrailingWhitespace: Trailing whitespace detected.",
-            "Put a comma after the last parameter of a multiline method call.",
-          ],
-          source: "    violation.fetch(:source) ",
-        )
-        suggest_changes = described_class.new(violation)
-
-        result = suggest_changes.call
-
-        expect(result).to eq <<~COMMENT.chomp
-          Layout/TrailingWhitespace: Trailing whitespace detected.<br>Put a comma after the last parameter of a multiline method call.<br>
-          ```suggestion
-              violation.fetch(:source)
-          ```
-        COMMENT
+    context "when suggested changes are disabled" do
+      context "single violation" do
+        it "does not make suggestion" do
+          violation = instance_double(
+            "Violation",
+            messages: [
+              "Put a comma after the last parameter of a multiline method call.",
+            ],
+            source: "    violation.fetch(:source)",
+            owner: double,
+          )
+          suggest_changes = described_class.new(violation)
+          flipper_feature = double
+          stub_const("Flipper", { suggested_changes: flipper_feature })
+          allow(flipper_feature).to(
+            receive(:enabled?).with(violation.owner).and_return(false)
+          )
+
+          result = suggest_changes.call
+
+          expect(result).to eq <<~COMMENT.chomp
+            Put a comma after the last parameter of a multiline method call.
+          COMMENT
+        end
       end
     end
   end