Skip to content

Commit

Permalink
Feature flag suggested changes
Browse files Browse the repository at this point in the history
  • Loading branch information
salbertson committed Apr 1, 2020
1 parent d48f119 commit fff3d3c
Show file tree
Hide file tree
Showing 11 changed files with 208 additions and 86 deletions.
1 change: 1 addition & 0 deletions .env
Original file line number Diff line number Diff line change
Expand Up @@ -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
3 changes: 3 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
12 changes: 12 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -383,6 +392,9 @@ DEPENDENCIES
email_validator
factory_girl_rails
faraday
flipper
flipper-active_record
flipper-ui
foreman
haml-rails (~> 2.0)
high_voltage
Expand Down
4 changes: 4 additions & 0 deletions app/models/owner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ def metered_plan?
whitelisted?
end

def flipper_id
"Owner:#{id}"
end

private

def hound_config
Expand Down
4 changes: 4 additions & 0 deletions app/models/violation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
20 changes: 12 additions & 8 deletions app/services/suggest_changes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
8 changes: 8 additions & 0 deletions config/initializers/flipper.rb
Original file line number Diff line number Diff line change
@@ -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
10 changes: 10 additions & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
22 changes: 22 additions & 0 deletions db/migrate/20200401183732_create_flipper_tables.rb
Original file line number Diff line number Diff line change
@@ -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
20 changes: 18 additions & 2 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
190 changes: 114 additions & 76 deletions spec/services/suggest_changes_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit fff3d3c

Please sign in to comment.