From dfd25c2a1448cd52adac96e1c5d310f3cdb58753 Mon Sep 17 00:00:00 2001 From: Yuri Bocharov Date: Sun, 7 Jul 2024 15:17:37 -0400 Subject: [PATCH] Feat/replace old feature flags with flipper (#5852) * feat: replace hand rolled feature flags with flipper * feat: use devise for auth --- app/assets/stylesheets/application.scss | 3 +- .../feature_flags_controller.rb | 10 ------- app/javascript/all_casa_admin.js | 1 - .../src/all_casa_admin/feature_flags.js | 30 ------------------- app/models/feature_flag.rb | 18 ----------- app/policies/case_contact_policy.rb | 2 +- app/services/feature_flag_service.rb | 16 ---------- .../feature_flags/index.html.erb | 18 ----------- app/views/casa_org/edit.html.erb | 2 +- .../layouts/_all_casa_admin_sidebar.html.erb | 4 +-- config/initializers/all_casa_admin_access.rb | 6 ++++ config/routes.rb | 24 ++++++++------- .../20240622020203_drop_feature_flags.rb | 15 ++++++++++ db/schema.rb | 10 +------ ...05830_enable_show_additional_expenses.rake | 13 -------- spec/factories/feature_flags.rb | 6 ---- spec/models/feature_flag_spec.rb | 14 --------- .../all_casa_admins/feature_flags_spec.rb | 26 ---------------- spec/services/feature_flag_service_spec.rb | 11 ------- .../all_casa_admins/sessions/new_spec.rb | 9 ++++++ .../case_contacts/additional_expenses_spec.rb | 2 +- spec/system/case_contacts/create_spec.rb | 2 +- spec/system/case_contacts/new_spec.rb | 3 +- .../feature_flags/index.html.erb_spec.rb | 20 ------------- spec/views/casa_orgs/edit.html.erb_spec.rb | 4 +-- 25 files changed, 55 insertions(+), 214 deletions(-) delete mode 100644 app/controllers/all_casa_admins/feature_flags_controller.rb delete mode 100644 app/javascript/src/all_casa_admin/feature_flags.js delete mode 100644 app/models/feature_flag.rb delete mode 100644 app/services/feature_flag_service.rb delete mode 100644 app/views/all_casa_admins/feature_flags/index.html.erb create mode 100644 config/initializers/all_casa_admin_access.rb create mode 100644 db/migrate/20240622020203_drop_feature_flags.rb delete mode 100644 lib/tasks/deployment/20220825005830_enable_show_additional_expenses.rake delete mode 100644 spec/factories/feature_flags.rb delete mode 100644 spec/models/feature_flag_spec.rb delete mode 100644 spec/requests/all_casa_admins/feature_flags_spec.rb delete mode 100644 spec/services/feature_flag_service_spec.rb delete mode 100644 spec/views/all_casa_admins/feature_flags/index.html.erb_spec.rb diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index cd122331ad..2be96e7119 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -37,7 +37,6 @@ @use "pages/case_contacts_form"; @use "pages/court_dates"; @use "pages/emancipation"; -@use "pages/feature_flags"; @use "pages/login"; @use "pages/password"; @use "pages/patch_notes"; @@ -45,5 +44,5 @@ @use "pages/supervisors"; @use "pages/volunteers"; -@use 'trix/dist/trix.css'; +@use "trix/dist/trix.css"; @use "actiontext"; diff --git a/app/controllers/all_casa_admins/feature_flags_controller.rb b/app/controllers/all_casa_admins/feature_flags_controller.rb deleted file mode 100644 index c1d75f76ed..0000000000 --- a/app/controllers/all_casa_admins/feature_flags_controller.rb +++ /dev/null @@ -1,10 +0,0 @@ -class AllCasaAdmins::FeatureFlagsController < AllCasaAdminsController - def index - @feature_flags = FeatureFlag.all.order(name: "asc") - end - - def update - @feature_flag = FeatureFlag.find(params[:id]) - @feature_flag.update(enabled: !@feature_flag.enabled) - end -end diff --git a/app/javascript/all_casa_admin.js b/app/javascript/all_casa_admin.js index 97d90c31ef..6324ded939 100644 --- a/app/javascript/all_casa_admin.js +++ b/app/javascript/all_casa_admin.js @@ -1,5 +1,4 @@ require('./src/all_casa_admin/tables') require('./src/all_casa_admin/patch_notes') -require('./src/all_casa_admin/feature_flags') require('./src/session_timeout_poller.js') require('./src/display_app_metric.js') diff --git a/app/javascript/src/all_casa_admin/feature_flags.js b/app/javascript/src/all_casa_admin/feature_flags.js deleted file mode 100644 index e72d0419e9..0000000000 --- a/app/javascript/src/all_casa_admin/feature_flags.js +++ /dev/null @@ -1,30 +0,0 @@ -$(function () { - const checkboxes = $('.form-check-input.toggle-switch') - - checkboxes.on('change', function (e) { - e.preventDefault() - const featureFlagId = $(this).data('feature-flag-id') - const uniqueCheckbox = $('#feature-checkbox-' + featureFlagId) - const csrfToken = $('meta[name=csrf-token]').attr('content') - const isChecked = this.checked - toggleSwitch(uniqueCheckbox, csrfToken, featureFlagId, isChecked) - }) - - function toggleSwitch (uniqueCheckbox, csrfToken, featureFlagId, checked) { - $.ajax({ - url: `/all_casa_admins/feature_flags/${featureFlagId}`, - method: 'PATCH', - contentType: 'application/json', - headers: { - 'X-CSRF-Token': csrfToken - }, - success: function () { - if (checked) { - uniqueCheckbox.prop('checked', true) - } else { - uniqueCheckbox.prop('checked', false) - } - } - }) - } -}) diff --git a/app/models/feature_flag.rb b/app/models/feature_flag.rb deleted file mode 100644 index 5b996ba80a..0000000000 --- a/app/models/feature_flag.rb +++ /dev/null @@ -1,18 +0,0 @@ -class FeatureFlag < ApplicationRecord - validates :name, presence: true, uniqueness: true -end - -# == Schema Information -# -# Table name: feature_flags -# -# id :bigint not null, primary key -# enabled :boolean default(FALSE), not null -# name :string not null -# created_at :datetime not null -# updated_at :datetime not null -# -# Indexes -# -# index_feature_flags_on_name (name) UNIQUE -# diff --git a/app/policies/case_contact_policy.rb b/app/policies/case_contact_policy.rb index e64815b589..d406307299 100644 --- a/app/policies/case_contact_policy.rb +++ b/app/policies/case_contact_policy.rb @@ -12,7 +12,7 @@ def destroy? end def additional_expenses_allowed? - FeatureFlagService.is_enabled?(FeatureFlagService::SHOW_ADDITIONAL_EXPENSES_FLAG) && + Flipper.enabled?(:show_additional_expenses) && current_organization.additional_expenses_enabled end diff --git a/app/services/feature_flag_service.rb b/app/services/feature_flag_service.rb deleted file mode 100644 index 0847197394..0000000000 --- a/app/services/feature_flag_service.rb +++ /dev/null @@ -1,16 +0,0 @@ -class FeatureFlagService - SOME_FLAG = "some_flag" - SHOW_ADDITIONAL_EXPENSES_FLAG = "show_additional_expenses" - - def self.is_enabled?(feature_flag_name) - FeatureFlag.find_by(name: feature_flag_name)&.enabled - end - - def self.enable!(feature_flag_name) - FeatureFlag.find_or_create_by(name: feature_flag_name).update!(enabled: true) - end - - def self.disable!(feature_flag_name) - FeatureFlag.find_or_create_by(name: feature_flag_name).update!(enabled: false) - end -end diff --git a/app/views/all_casa_admins/feature_flags/index.html.erb b/app/views/all_casa_admins/feature_flags/index.html.erb deleted file mode 100644 index 8e19f73c20..0000000000 --- a/app/views/all_casa_admins/feature_flags/index.html.erb +++ /dev/null @@ -1,18 +0,0 @@ -
-

Feature Flags

- - - <% @feature_flags.each do |feature_flag| %> - - - - - <% end %> - -
<%= feature_flag.name %> -
- checked <% end %>> - -
-
-
diff --git a/app/views/casa_org/edit.html.erb b/app/views/casa_org/edit.html.erb index 7805b8250f..75a1cc4c66 100644 --- a/app/views/casa_org/edit.html.erb +++ b/app/views/casa_org/edit.html.erb @@ -80,7 +80,7 @@ <%# Twilio Form End %> - <% if FeatureFlagService.is_enabled?(FeatureFlagService::SHOW_ADDITIONAL_EXPENSES_FLAG) %> + <% if Flipper.enabled?(:show_additional_expenses) %>
<%= form.check_box :additional_expenses_enabled, class: 'form-check-input' %> <%= form.label :additional_expenses_enabled, "Volunteers can add Other Expenses", class: 'form-check-label mb-2' %> diff --git a/app/views/layouts/_all_casa_admin_sidebar.html.erb b/app/views/layouts/_all_casa_admin_sidebar.html.erb index e029362731..0984d2cc6e 100644 --- a/app/views/layouts/_all_casa_admin_sidebar.html.erb +++ b/app/views/layouts/_all_casa_admin_sidebar.html.erb @@ -22,8 +22,8 @@ Edit Profile <% end %> -
  • - <%= link_to all_casa_admins_feature_flags_path do %> +
  • + <%= link_to "/flipper" do %> Feature Flags <% end %> diff --git a/config/initializers/all_casa_admin_access.rb b/config/initializers/all_casa_admin_access.rb new file mode 100644 index 0000000000..46d1cbb416 --- /dev/null +++ b/config/initializers/all_casa_admin_access.rb @@ -0,0 +1,6 @@ +class CanAccessFlipperUI + def self.matches?(request) + session = request.env["warden"].raw_session.to_h + session["warden.user.all_casa_admin.session"].present? + end +end diff --git a/config/routes.rb b/config/routes.rb index cba2c83874..58441f5103 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,19 +1,19 @@ # frozen_string_literal: true -def set_up_flipper - flipper_app = Flipper::UI.app(Flipper.instance, rack_protection: {except: :http_origin}) do |builder| - builder.use Rack::Auth::Basic do |username, password| - username == ENV["FLIPPER_USERNAME"] && password == ENV["FLIPPER_PASSWORD"] - end - end - mount flipper_app, at: "/flipper" -end +# def set_up_flipper +# flipper_app = Flipper::UI.app(Flipper.instance, rack_protection: {except: :http_origin}) do |builder| +# builder.use Rack::Auth::Basic do |username, password| +# username == ENV["FLIPPER_USERNAME"] && password == ENV["FLIPPER_PASSWORD"] +# end +# end +# mount flipper_app, at: "/flipper" +# end Rails.application.routes.draw do mount Rswag::Ui::Engine => "/api-docs" mount Rswag::Api::Engine => "/api-docs" - set_up_flipper + # set_up_flipper devise_for :all_casa_admins, path: "all_casa_admins", controllers: {sessions: "all_casa_admins/sessions"} devise_for :users, controllers: {sessions: "users/sessions", passwords: "users/passwords"} @@ -201,8 +201,6 @@ def set_up_flipper end resources :patch_notes, only: %i[create destroy index update] - - resources :feature_flags, only: %i[index update] end resources :all_casa_admins, only: [:new, :create] do @@ -245,4 +243,8 @@ def set_up_flipper end end end + + constraints CanAccessFlipperUI do + mount Flipper::UI.app(Flipper) => "/flipper" + end end diff --git a/db/migrate/20240622020203_drop_feature_flags.rb b/db/migrate/20240622020203_drop_feature_flags.rb new file mode 100644 index 0000000000..2910f23496 --- /dev/null +++ b/db/migrate/20240622020203_drop_feature_flags.rb @@ -0,0 +1,15 @@ +class DropFeatureFlags < ActiveRecord::Migration[7.1] + def up + drop_table :feature_flags + end + + def down + create_table :feature_flags do |t| + t.string :name, null: false + t.boolean :enabled, null: false, default: false + t.timestamps + end + + add_index :feature_flags, :name, unique: true, algorithm: :concurrently + end +end diff --git a/db/schema.rb b/db/schema.rb index d36f23a232..3c91d8bac1 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2024_06_21_165358) do +ActiveRecord::Schema[7.1].define(version: 2024_06_22_020203) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -332,14 +332,6 @@ t.index ["emancipation_category_id"], name: "index_emancipation_options_on_emancipation_category_id" end - create_table "feature_flags", force: :cascade do |t| - t.string "name", null: false - t.boolean "enabled", default: false, null: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.index ["name"], name: "index_feature_flags_on_name", unique: true - end - create_table "flipper_features", force: :cascade do |t| t.string "key", null: false t.datetime "created_at", null: false diff --git a/lib/tasks/deployment/20220825005830_enable_show_additional_expenses.rake b/lib/tasks/deployment/20220825005830_enable_show_additional_expenses.rake deleted file mode 100644 index a3372becae..0000000000 --- a/lib/tasks/deployment/20220825005830_enable_show_additional_expenses.rake +++ /dev/null @@ -1,13 +0,0 @@ -namespace :after_party do - desc "Deployment task: this task enable to show additional expenses" - task enable_show_additional_expenses: :environment do - puts "Running deploy task 'enable_show_additional_expenses'" - - FeatureFlagService.enable!(FeatureFlagService::SHOW_ADDITIONAL_EXPENSES_FLAG) - - # Update task as completed. If you remove the line below, the task will - # run with every deploy (or every time you call after_party:run). - AfterParty::TaskRecord - .create version: AfterParty::TaskRecorder.new(__FILE__).timestamp - end -end diff --git a/spec/factories/feature_flags.rb b/spec/factories/feature_flags.rb deleted file mode 100644 index f398c01c26..0000000000 --- a/spec/factories/feature_flags.rb +++ /dev/null @@ -1,6 +0,0 @@ -FactoryBot.define do - factory :feature_flag do - sequence(:name) { |n| "feature_name #{n}" } - enabled { true } - end -end diff --git a/spec/models/feature_flag_spec.rb b/spec/models/feature_flag_spec.rb deleted file mode 100644 index 35e797420f..0000000000 --- a/spec/models/feature_flag_spec.rb +++ /dev/null @@ -1,14 +0,0 @@ -require "rails_helper" - -RSpec.describe FeatureFlag, type: :model do - context "When creating a new feature flag" do - it "raises an exception for duplicated feature flag name" do - duplicate_feature_flag_name = "feature name" - - expect { - create(:feature_flag, name: duplicate_feature_flag_name) - create(:feature_flag, name: duplicate_feature_flag_name) - }.to raise_error(ActiveRecord::RecordInvalid) - end - end -end diff --git a/spec/requests/all_casa_admins/feature_flags_spec.rb b/spec/requests/all_casa_admins/feature_flags_spec.rb deleted file mode 100644 index 77d8ffbf19..0000000000 --- a/spec/requests/all_casa_admins/feature_flags_spec.rb +++ /dev/null @@ -1,26 +0,0 @@ -require "rails_helper" - -RSpec.describe "/all_casa_admins/feature_flags", type: :request do - let(:all_casa_admin) { build(:all_casa_admin) } - let(:feature_flag) { create(:feature_flag) } - - before(:each) { sign_in all_casa_admin } - - describe "GET /index" do - it "gets all the feature flags" do - get all_casa_admins_feature_flags_path - - expect(response).to render_template(:index) - expect(response).to be_successful - end - end - - describe "PATCH /update" do - it "updates enable to false on toggle" do - patch "/all_casa_admins/feature_flags/#{feature_flag.id}" - - feature_flag.reload - expect(feature_flag.enabled).to eq(false) - end - end -end diff --git a/spec/services/feature_flag_service_spec.rb b/spec/services/feature_flag_service_spec.rb deleted file mode 100644 index 0f754fc7b4..0000000000 --- a/spec/services/feature_flag_service_spec.rb +++ /dev/null @@ -1,11 +0,0 @@ -require "rails_helper" - -RSpec.describe FeatureFlagService do - it "can be enabled and disabled" do - expect(described_class.is_enabled?(FeatureFlagService::SOME_FLAG)).to be_falsey - described_class.enable!(FeatureFlagService::SOME_FLAG) - expect(described_class.is_enabled?(FeatureFlagService::SOME_FLAG)).to be_truthy - described_class.disable!(FeatureFlagService::SOME_FLAG) - expect(described_class.is_enabled?(FeatureFlagService::SOME_FLAG)).to be_falsey - end -end diff --git a/spec/system/all_casa_admins/sessions/new_spec.rb b/spec/system/all_casa_admins/sessions/new_spec.rb index cf970c86a4..ca14d66baa 100644 --- a/spec/system/all_casa_admins/sessions/new_spec.rb +++ b/spec/system/all_casa_admins/sessions/new_spec.rb @@ -19,6 +19,11 @@ expect(page).to have_text "Signed out successfully" expect(page).to have_text "All CASA Log In" end + + it "allows access to flipper" do + visit "/flipper" + expect(page).to have_text "Flipper" + end end context "when unauthenticated" do @@ -50,5 +55,9 @@ expect(page).to have_text "Invalid Email or password" end + + it "denies access to flipper" do + expect { visit "/flipper" }.to raise_error(ActionController::RoutingError) + end end end diff --git a/spec/system/case_contacts/additional_expenses_spec.rb b/spec/system/case_contacts/additional_expenses_spec.rb index b5ff7e104c..6f5e3a8da8 100644 --- a/spec/system/case_contacts/additional_expenses_spec.rb +++ b/spec/system/case_contacts/additional_expenses_spec.rb @@ -7,7 +7,7 @@ let(:casa_case) { create(:casa_case, casa_org: organization) } before do - FeatureFlagService.enable!(FeatureFlagService::SHOW_ADDITIONAL_EXPENSES_FLAG) + allow(Flipper).to receive(:enabled?).with(:show_additional_expenses).and_return(true) create(:case_assignment, casa_case: casa_case, volunteer: volunteer) end diff --git a/spec/system/case_contacts/create_spec.rb b/spec/system/case_contacts/create_spec.rb index dcbbf1a82f..826a0bb759 100644 --- a/spec/system/case_contacts/create_spec.rb +++ b/spec/system/case_contacts/create_spec.rb @@ -126,7 +126,7 @@ context "when the org has neither reimbursable expenses nor travel" do before do - FeatureFlagService.disable!(FeatureFlagService::SHOW_ADDITIONAL_EXPENSES_FLAG) + allow(Flipper).to receive(:enabled?).with(:show_additional_expenses).and_return(false) allow_any_instance_of(CasaOrg).to receive(:show_driving_reimbursement).and_return(false) sign_in volunteer end diff --git a/spec/system/case_contacts/new_spec.rb b/spec/system/case_contacts/new_spec.rb index b6c6e5400d..ac700d7128 100644 --- a/spec/system/case_contacts/new_spec.rb +++ b/spec/system/case_contacts/new_spec.rb @@ -49,10 +49,11 @@ before(:each) do sign_in volunteer + allow(Flipper).to receive(:enabled?).with(:show_additional_expenses).and_return(true) end it "is successful without a. Miles Driven or driving reimbursement", js: true do - FeatureFlagService.enable!(FeatureFlagService::SHOW_ADDITIONAL_EXPENSES_FLAG) + allow(Flipper).to receive(:enabled?).with(:show_additional_expenses).and_return(false) organization = build(:casa_org) build(:contact_type_group, name: "Empty", casa_org: organization) grp_with_hidden = build(:contact_type_group, name: "OnlyHiddenTypes", casa_org: organization) diff --git a/spec/views/all_casa_admins/feature_flags/index.html.erb_spec.rb b/spec/views/all_casa_admins/feature_flags/index.html.erb_spec.rb deleted file mode 100644 index dba6ba9085..0000000000 --- a/spec/views/all_casa_admins/feature_flags/index.html.erb_spec.rb +++ /dev/null @@ -1,20 +0,0 @@ -require "rails_helper" - -RSpec.describe "patch_notes/index", type: :view do - let(:all_casa_admin) { build(:all_casa_admin) } - - before(:each) { sign_in all_casa_admin } - - describe "the feature flag list" do - it "displays the feature flags with toggles" do - feature_flag_1 = create(:feature_flag, name: "Test feature one") - feature_flag_2 = create(:feature_flag, name: "Test feature two") - assign(:feature_flags, [feature_flag_1, feature_flag_2]) - - render template: "all_casa_admins/feature_flags/index" - - expect(rendered).to include("Test feature one") - expect(rendered).to include("Test feature two") - end - end -end diff --git a/spec/views/casa_orgs/edit.html.erb_spec.rb b/spec/views/casa_orgs/edit.html.erb_spec.rb index fccc63d40e..c0a64e1851 100644 --- a/spec/views/casa_orgs/edit.html.erb_spec.rb +++ b/spec/views/casa_orgs/edit.html.erb_spec.rb @@ -145,7 +145,7 @@ describe "additional expense feature flag" do context "enabled" do it "has option to enable additional expenses" do - FeatureFlagService.enable!(FeatureFlagService::SHOW_ADDITIONAL_EXPENSES_FLAG) + allow(Flipper).to receive(:enabled?).with(:show_additional_expenses).and_return(true) organization = build_stubbed(:casa_org) allow(view).to receive(:current_organization).and_return(organization) @@ -157,7 +157,7 @@ context "disabled" do it "has option to enable additional expenses" do - FeatureFlagService.disable!(FeatureFlagService::SHOW_ADDITIONAL_EXPENSES_FLAG) + allow(Flipper).to receive(:enabled?).with(:show_additional_expenses).and_return(false) organization = build_stubbed(:casa_org) allow(view).to receive(:current_organization).and_return(organization)