From 1b4ca38f5dd974b3345a04d118b89c7b60132458 Mon Sep 17 00:00:00 2001
From: Vladimir Shushlin <v.shushlin@gmail.com>
Date: Thu, 7 Feb 2019 14:42:57 +0300
Subject: [PATCH 1/7] Add audit logs for feature flags

* Move feature flag CRUD into service objects
* Create audit logs FF CRUD
* Calculate environment scope difference and add audit logs for changed
* scopes on update
* Add specs
* Use custom_message in feature flag audit events
* feature flags html actions and @feature_flag variable
---
 .../projects/feature_flags_controller.rb      |  41 +++--
 ee/app/services/feature_flags/base_service.rb |  32 ++++
 .../services/feature_flags/create_service.rb  |  32 ++++
 .../services/feature_flags/destroy_service.rb |  17 ++
 .../services/feature_flags/update_service.rb  |  74 +++++++++
 .../unreleased/feature-flags-audit-events.yml |   5 +
 .../projects/feature_flags_controller_spec.rb |  41 -----
 .../user_creates_feature_flag_spec.rb         |   7 +
 .../user_deletes_feature_flag_spec.rb         |  37 +++++
 .../user_updates_feature_flag_spec.rb         |   9 ++
 .../feature_flags/create_service_spec.rb      |  51 ++++++
 .../feature_flags/destroy_service_spec.rb     |  33 ++++
 .../feature_flags/update_service_spec.rb      | 147 ++++++++++++++++++
 13 files changed, 464 insertions(+), 62 deletions(-)
 create mode 100644 ee/app/services/feature_flags/base_service.rb
 create mode 100644 ee/app/services/feature_flags/create_service.rb
 create mode 100644 ee/app/services/feature_flags/destroy_service.rb
 create mode 100644 ee/app/services/feature_flags/update_service.rb
 create mode 100644 ee/changelogs/unreleased/feature-flags-audit-events.yml
 create mode 100644 ee/spec/features/projects/feature_flags/user_deletes_feature_flag_spec.rb
 create mode 100644 ee/spec/services/feature_flags/create_service_spec.rb
 create mode 100644 ee/spec/services/feature_flags/destroy_service_spec.rb
 create mode 100644 ee/spec/services/feature_flags/update_service_spec.rb

diff --git a/ee/app/controllers/projects/feature_flags_controller.rb b/ee/app/controllers/projects/feature_flags_controller.rb
index 009d3c4f214092..ed8329de89b78c 100644
--- a/ee/app/controllers/projects/feature_flags_controller.rb
+++ b/ee/app/controllers/projects/feature_flags_controller.rb
@@ -28,7 +28,6 @@ def index
   end
 
   def new
-    @feature_flag = project.operations_feature_flags.new
   end
 
   def show
@@ -36,23 +35,21 @@ def show
       format.json do
         Gitlab::PollingInterval.set_header(response, interval: 10_000)
 
-        render_success_json
+        render_success_json(feature_flag)
       end
     end
   end
 
   def create
-    @feature_flag = project.operations_feature_flags.create(create_params)
+    result = FeatureFlags::CreateService.new(project, current_user, create_params).execute
 
-    if @feature_flag.persisted?
+    if result[:status] == :success
       respond_to do |format|
-        format.html { redirect_to_index(notice: 'Feature flag was successfully created.') }
-        format.json { render_success_json }
+        format.json { render_success_json(result[:feature_flag]) }
       end
     else
       respond_to do |format|
-        format.html { render :new }
-        format.json { render_error_json }
+        format.json { render_error_json(result[:message]) }
       end
     end
   end
@@ -61,29 +58,31 @@ def edit
   end
 
   def update
-    if feature_flag.update(update_params)
+    result = FeatureFlags::UpdateService.new(project, current_user, update_params).execute(feature_flag)
+
+    if result[:status] == :success
       respond_to do |format|
-        format.html { redirect_to_index(notice: 'Feature flag was successfully updated.') }
-        format.json { render_success_json }
+        format.json { render_success_json(result[:feature_flag]) }
       end
     else
       respond_to do |format|
-        format.html { render :edit }
-        format.json { render_error_json }
+        format.json { render_error_json(result[:message]) }
       end
     end
   end
 
   def destroy
-    if feature_flag.destroy
+    result = FeatureFlags::DestroyService.new(project, current_user).execute(feature_flag)
+
+    if result[:status] == :success
       respond_to do |format|
         format.html { redirect_to_index(notice: 'Feature flag was successfully removed.') }
-        format.json { render_success_json }
+        format.json { render_success_json(feature_flag) }
       end
     else
       respond_to do |format|
         format.html { redirect_to_index(alert: 'Feature flag was not removed.') }
-        format.json { render_error_json }
+        format.json { render_error_json(result[:message]) }
       end
     end
   end
@@ -106,7 +105,7 @@ def update_params
                   scopes_attributes: [:id, :environment_scope, :active, :_destroy])
   end
 
-  def feature_flag_json
+  def feature_flag_json(feature_flag)
     FeatureFlagSerializer
       .new(project: @project, current_user: @current_user)
       .represent(feature_flag)
@@ -129,12 +128,12 @@ def redirect_to_index(**args)
     redirect_to project_feature_flags_path(@project), status: :found, **args
   end
 
-  def render_success_json
-    render json: feature_flag_json, status: :ok
+  def render_success_json(feature_flag)
+    render json: feature_flag_json(feature_flag), status: :ok
   end
 
-  def render_error_json
-    render json: { message: feature_flag.errors.full_messages },
+  def render_error_json(messages)
+    render json: { message: messages },
            status: :bad_request
   end
 end
diff --git a/ee/app/services/feature_flags/base_service.rb b/ee/app/services/feature_flags/base_service.rb
new file mode 100644
index 00000000000000..fc8c03d67a47f4
--- /dev/null
+++ b/ee/app/services/feature_flags/base_service.rb
@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+module FeatureFlags
+  class BaseService < ::BaseService
+    AUDITABLE_ATTRIBUTES = %w(name description).freeze
+
+    protected
+
+    def log_audit_event(feature_flag, message)
+      return unless message
+
+      details =
+        {
+          custom_message: message,
+          target_id: feature_flag.id,
+          target_type: feature_flag.class.name,
+          target_details: feature_flag.name
+        }
+
+      ::AuditEventService.new(
+        current_user,
+        feature_flag.project,
+        details
+      ).security_event
+    end
+
+    def created_scope_message(scope)
+      "Created rule <strong>#{scope.environment_scope}</strong> "\
+      "and set it as <strong>#{scope.active ? "active" : "inactive"}</strong>."
+    end
+  end
+end
diff --git a/ee/app/services/feature_flags/create_service.rb b/ee/app/services/feature_flags/create_service.rb
new file mode 100644
index 00000000000000..d6a31eb3213cc0
--- /dev/null
+++ b/ee/app/services/feature_flags/create_service.rb
@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+module FeatureFlags
+  class CreateService < FeatureFlags::BaseService
+    def execute
+      ActiveRecord::Base.transaction do
+        feature_flag = project.operations_feature_flags.new(params)
+
+        if feature_flag.save
+          log_audit_event(feature_flag, audit_message(feature_flag))
+
+          success(feature_flag: feature_flag)
+        else
+          error(feature_flag.errors.full_messages)
+        end
+      end
+    end
+
+    private
+
+    def audit_message(feature_flag)
+      message_parts = ["Created feature flag <strong>#{feature_flag.name}</strong>",
+                       "with description <strong>\"#{feature_flag.description}\"</strong>."]
+
+      message_parts += feature_flag.scopes.map do |scope|
+        created_scope_message(scope)
+      end
+
+      message_parts.join(" ")
+    end
+  end
+end
diff --git a/ee/app/services/feature_flags/destroy_service.rb b/ee/app/services/feature_flags/destroy_service.rb
new file mode 100644
index 00000000000000..d7c7c91cc0f161
--- /dev/null
+++ b/ee/app/services/feature_flags/destroy_service.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+module FeatureFlags
+  class DestroyService < FeatureFlags::BaseService
+    def execute(feature_flag)
+      ActiveRecord::Base.transaction do
+        if feature_flag.destroy
+          log_audit_event(feature_flag, "Deleted feature flag <strong>#{feature_flag.name}</strong>.")
+
+          success(feature_flag: feature_flag)
+        else
+          error(feature_flag.errors.full_messages)
+        end
+      end
+    end
+  end
+end
diff --git a/ee/app/services/feature_flags/update_service.rb b/ee/app/services/feature_flags/update_service.rb
new file mode 100644
index 00000000000000..64afd91eedf37b
--- /dev/null
+++ b/ee/app/services/feature_flags/update_service.rb
@@ -0,0 +1,74 @@
+# frozen_string_literal: true
+
+module FeatureFlags
+  class UpdateService < FeatureFlags::BaseService
+    AUDITABLE_SCOPE_ATTRIBUTES_HUMAN_NAMES = {
+      'active' => 'active state',
+      'environment_scope' => 'environment scope'
+    }.freeze
+
+    def execute(feature_flag)
+      ActiveRecord::Base.transaction do
+        feature_flag.assign_attributes(params)
+
+        audit_log = audit_message(feature_flag)
+
+        if feature_flag.save
+          log_audit_event(feature_flag, audit_log)
+
+          success(feature_flag: feature_flag)
+        else
+          error(feature_flag.errors.full_messages)
+        end
+      end
+    end
+
+    private
+
+    def audit_message(feature_flag)
+      changes = changed_attributes_messages(feature_flag)
+      changes += changed_scopes_messages(feature_flag)
+
+      return if changes.empty?
+
+      "Updated feature flag <strong>#{feature_flag.name}</strong>. " + changes.join(" ")
+    end
+
+    def changed_attributes_messages(feature_flag)
+      feature_flag.changes.slice(*AUDITABLE_ATTRIBUTES).map do |attribute_name, changes|
+        "Updated #{attribute_name} "\
+        "from <strong>\"#{changes.first}\"</strong> to "\
+        "<strong>\"#{changes.second}\"</strong>."
+      end
+    end
+
+    def changed_scopes_messages(feature_flag)
+      feature_flag.scopes.map do |scope|
+        if scope.new_record?
+          created_scope_message(scope)
+        elsif scope.marked_for_destruction?
+          deleted_scope_message(scope)
+        else
+          updated_scope_message(scope)
+        end
+      end.compact # updated_scope_message can return nil if nothing has been changed
+    end
+
+    def deleted_scope_message(scope)
+      "Deleted rule <strong>#{scope.environment_scope}</strong>."
+    end
+
+    def updated_scope_message(scope)
+      changes = scope.changes.slice(*AUDITABLE_SCOPE_ATTRIBUTES_HUMAN_NAMES.keys)
+      return if changes.empty?
+
+      message = "Updated rule <strong>#{scope.environment_scope}</strong> "
+      message += changes.map do |attribute_name, change|
+        name = AUDITABLE_SCOPE_ATTRIBUTES_HUMAN_NAMES[attribute_name]
+        "#{name} from <strong>#{change.first}</strong> to <strong>#{change.second}</strong>"
+      end.join(' ')
+
+      message + '.'
+    end
+  end
+end
diff --git a/ee/changelogs/unreleased/feature-flags-audit-events.yml b/ee/changelogs/unreleased/feature-flags-audit-events.yml
new file mode 100644
index 00000000000000..b7353e5aebd898
--- /dev/null
+++ b/ee/changelogs/unreleased/feature-flags-audit-events.yml
@@ -0,0 +1,5 @@
+---
+title: Add audit log for managing feature flags
+merge_request: 9487
+author:
+type: added
diff --git a/ee/spec/controllers/projects/feature_flags_controller_spec.rb b/ee/spec/controllers/projects/feature_flags_controller_spec.rb
index e1f5744b1018c0..5eac7373464588 100644
--- a/ee/spec/controllers/projects/feature_flags_controller_spec.rb
+++ b/ee/spec/controllers/projects/feature_flags_controller_spec.rb
@@ -300,24 +300,6 @@
     end
   end
 
-  describe 'POST create' do
-    render_views
-
-    subject { post(:create, params: params) }
-
-    context 'when creating a new feature flag' do
-      let(:params) do
-        view_params.merge(operations_feature_flag: { name: 'my_feature_flag', active: true })
-      end
-
-      it 'creates and redirects to list' do
-        subject
-
-        expect(response).to redirect_to(project_feature_flags_path(project))
-      end
-    end
-  end
-
   describe 'POST create.json' do
     subject { post(:create, params: params, format: :json) }
 
@@ -435,29 +417,6 @@
     end
   end
 
-  describe 'PUT update' do
-    let!(:feature_flag) { create(:operations_feature_flag, project: project, name: 'my_feature_flag') }
-
-    render_views
-
-    subject { put(:update, params: params) }
-
-    context 'when updating an existing feature flag' do
-      let(:params) do
-        view_params.merge(
-          id: feature_flag.id,
-          operations_feature_flag: { name: 'my_feature_flag_v2', active: true }
-        )
-      end
-
-      it 'updates and redirects to list' do
-        subject
-
-        expect(response).to redirect_to(project_feature_flags_path(project))
-      end
-    end
-  end
-
   describe 'DELETE destroy.json' do
     subject { delete(:destroy, params: params, format: :json) }
 
diff --git a/ee/spec/features/projects/feature_flags/user_creates_feature_flag_spec.rb b/ee/spec/features/projects/feature_flags/user_creates_feature_flag_spec.rb
index ac62f2c330b688..0a3f0a1824062d 100644
--- a/ee/spec/features/projects/feature_flags/user_creates_feature_flag_spec.rb
+++ b/ee/spec/features/projects/feature_flags/user_creates_feature_flag_spec.rb
@@ -19,6 +19,7 @@
       visit(new_project_feature_flag_path(project))
       set_feature_flag_info('ci_live_trace', 'For live trace')
       click_button 'Create feature flag'
+      expect(page).to have_current_path(project_feature_flags_path(project))
     end
 
     it 'shows the created feature flag' do
@@ -32,6 +33,12 @@
         end
       end
     end
+
+    it 'records audit event' do
+      visit(project_audit_events_path(project))
+
+      expect(page).to have_text("Created feature flag ci live trace with description \"For live trace\".")
+    end
   end
 
   context 'when creates with disabling the default scope' do
diff --git a/ee/spec/features/projects/feature_flags/user_deletes_feature_flag_spec.rb b/ee/spec/features/projects/feature_flags/user_deletes_feature_flag_spec.rb
new file mode 100644
index 00000000000000..86dc6dbd31d80b
--- /dev/null
+++ b/ee/spec/features/projects/feature_flags/user_deletes_feature_flag_spec.rb
@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'User deletes feature flag', :js do
+  include FeatureFlagHelpers
+
+  let(:user) { create(:user) }
+  let(:project) { create(:project, namespace: user.namespace) }
+
+  let!(:feature_flag) do
+    create_flag(project, 'ci_live_trace', false,
+                description: 'For live trace feature')
+  end
+
+  before do
+    project.add_developer(user)
+    stub_licensed_features(feature_flags: true)
+    sign_in(user)
+
+    visit(project_feature_flags_path(project))
+
+    find('.js-feature-flag-delete-button').click
+    click_button('Delete feature flag')
+    expect(page).to have_current_path(project_feature_flags_path(project))
+  end
+
+  it 'user does not see feature flag' do
+    expect(page).to have_no_content('ci_live_trace')
+  end
+
+  it 'records audit event' do
+    visit(project_audit_events_path(project))
+
+    expect(page).to have_text("Deleted feature flag ci live trace.")
+  end
+end
diff --git a/ee/spec/features/projects/feature_flags/user_updates_feature_flag_spec.rb b/ee/spec/features/projects/feature_flags/user_updates_feature_flag_spec.rb
index 1b2fcee5a8b293..5d49da1519dc67 100644
--- a/ee/spec/features/projects/feature_flags/user_updates_feature_flag_spec.rb
+++ b/ee/spec/features/projects/feature_flags/user_updates_feature_flag_spec.rb
@@ -43,6 +43,7 @@
       end
 
       click_button 'Save changes'
+      expect(page).to have_current_path(project_feature_flags_path(project))
     end
 
     it 'shows the updated feature flag' do
@@ -58,6 +59,14 @@
         end
       end
     end
+
+    it 'records audit event' do
+      visit(project_audit_events_path(project))
+
+      expect(page).to(
+        have_text("Updated feature flag ci live trace. Updated rule review/* active state from true to false.")
+      )
+    end
   end
 
   context 'when user adds a new scope' do
diff --git a/ee/spec/services/feature_flags/create_service_spec.rb b/ee/spec/services/feature_flags/create_service_spec.rb
new file mode 100644
index 00000000000000..8328389d2d1747
--- /dev/null
+++ b/ee/spec/services/feature_flags/create_service_spec.rb
@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe FeatureFlags::CreateService do
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+
+  describe '#execute' do
+    subject do
+      described_class.new(project, user, params).execute
+    end
+    let(:status) { subject[:status] }
+    let(:feature_flag) { subject[:feature_flag] }
+    let(:errors) { subject[:message] }
+
+    context 'when feature flag can not be created' do
+      let(:params) { {} }
+      it { expect(status).to eq(:error) }
+      it { expect(errors).to include("Name can't be blank") }
+      it { expect { subject }.not_to change { AuditEvent.count } }
+    end
+
+    context 'when feature flag is saved correctly' do
+      let(:params) do
+        {
+          name: 'feature_flag',
+          description: 'description',
+          scopes_attributes: [{ environment_scope: '*', active: true },
+                              { environment_scope: 'production', active: false }]
+        }
+      end
+
+      it { expect(status).to eq(:success) }
+
+      it 'creates feature flag' do
+        expect { subject }.to change { Operations::FeatureFlag.count }.by(1)
+      end
+
+      it 'creates audit event' do
+        expected_message = "Created feature flag <strong>feature flag</strong> "\
+                           "with description <strong>\"description\"</strong>. "\
+                           "Created rule <strong>*</strong> and set it as <strong>active</strong>. "\
+                           "Created rule <strong>production</strong> and set it as <strong>inactive</strong>."
+
+        expect { subject }.to change { AuditEvent.count }.by(1)
+        expect(AuditEvent.last.present.action).to eq(expected_message)
+      end
+    end
+  end
+end
diff --git a/ee/spec/services/feature_flags/destroy_service_spec.rb b/ee/spec/services/feature_flags/destroy_service_spec.rb
new file mode 100644
index 00000000000000..7bcf6093bf726b
--- /dev/null
+++ b/ee/spec/services/feature_flags/destroy_service_spec.rb
@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe FeatureFlags::DestroyService do
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+  let!(:feature_flag) { create(:operations_feature_flag) }
+
+  describe '#execute' do
+    subject { described_class.new(project, user).execute(feature_flag) }
+    let(:status) { subject[:status] }
+    let(:audit_event) do
+      subject
+      AuditEvent.last
+    end
+    let(:audit_event_message) { audit_event.present.action }
+
+    it { expect(status).to eq(:success) }
+    it { expect { subject }.to change { Operations::FeatureFlag.count }.by(-1) }
+    it { expect { subject }.to change { AuditEvent.count }.by(1) }
+    it { expect(audit_event_message).to eq("Deleted feature flag <strong>#{feature_flag.name.tr('_', ' ')}</strong>.") }
+
+    context 'when feature flag can not be destroyed' do
+      before do
+        allow(feature_flag).to receive(:destroy).and_return(false)
+      end
+
+      it { expect(status).to eq(:error) }
+      it { expect { subject }.not_to change { AuditEvent.count } }
+    end
+  end
+end
diff --git a/ee/spec/services/feature_flags/update_service_spec.rb b/ee/spec/services/feature_flags/update_service_spec.rb
new file mode 100644
index 00000000000000..9d34d3535cc273
--- /dev/null
+++ b/ee/spec/services/feature_flags/update_service_spec.rb
@@ -0,0 +1,147 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe FeatureFlags::UpdateService do
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+  let(:feature_flag) { create(:operations_feature_flag) }
+
+  describe '#execute' do
+    subject { described_class.new(project, user, params).execute(feature_flag) }
+    let(:status) { subject[:status] }
+    let(:returned_feature_flag) { subject[:feature_flag] }
+    let(:errors) { subject[:message] }
+    let(:params) { { name: 'new_name' } }
+    let(:audit_event_message) do
+      subject
+      AuditEvent.last.present.action
+    end
+
+    it { expect(status).to eq(:success) }
+    it { expect(returned_feature_flag).to be_valid }
+    it { expect { subject }.to change { AuditEvent.count }.by(1) }
+    it 'creates audit event with correct message' do
+      name_was = feature_flag.name
+
+      expect(audit_event_message).to(
+        eq("Updated feature flag <strong>new name</strong>. "\
+           "Updated name from <strong>\"#{name_was.tr('_', ' ')}\"</strong> "\
+           "to <strong>\"new name\"</strong>.")
+      )
+    end
+
+    context 'with invalid params' do
+      let(:params) { { name: nil } }
+
+      it { expect(status).to eq(:error) }
+      it { expect(errors).to include("Name can't be blank") }
+      it { expect { subject }.not_to change { AuditEvent.count } }
+    end
+
+    context 'when nothing is changed' do
+      let(:params) { {} }
+
+      it { expect(status).to eq(:success) }
+      it { expect(returned_feature_flag).to be_valid }
+      it { expect { subject }.not_to change { AuditEvent.count } }
+    end
+
+    context 'description is being changed' do
+      let(:params) { { description: 'new description' } }
+
+      it 'creates audit event with changed description' do
+        expect(audit_event_message).to(
+          include("Updated description from <strong>\"\"</strong>"\
+                  " to <strong>\"new description\"</strong>.")
+        )
+      end
+    end
+
+    context 'when active state is changed' do
+      let(:params) do
+        {
+          scopes_attributes: [{ id: feature_flag.scopes.first.id, active: false }]
+        }
+      end
+
+      it 'creates audit event about changing active stae' do
+        expect(audit_event_message).to(
+          include("Updated rule <strong>*</strong> active state "\
+                  "from <strong>true</strong> to <strong>false</strong>.")
+        )
+      end
+    end
+
+    context 'when scope is renamed' do
+      let(:changed_scope) { feature_flag.scopes.create!(environment_scope: 'review', active: true) }
+      let(:new_environment_scope) { 'staging' }
+      let(:params) do
+        {
+          scopes_attributes: [{ id: changed_scope.id, environment_scope: new_environment_scope }]
+        }
+      end
+
+      it 'creates audit event with changed name' do
+        expect(audit_event_message).to(
+          include("Updated rule <strong>staging</strong> environment scope "\
+                  "from <strong>review</strong> to <strong>staging</strong>.")
+        )
+      end
+
+      context 'when scope can not be updated' do
+        let(:new_environment_scope) { '' }
+
+        it { expect { subject }.not_to change { AuditEvent.count } }
+        it { expect(status).to eq(:error) }
+      end
+    end
+
+    context 'when scope is deleted' do
+      let(:deleted_scope) { feature_flag.scopes.create!(environment_scope: 'review', active: true) }
+      let(:params) do
+        {
+          scopes_attributes: [{ id: deleted_scope.id, '_destroy': true }]
+        }
+      end
+
+      it { expect(audit_event_message).to include("Deleted rule <strong>review</strong>.") }
+
+      context 'when scope can not be deleted' do
+        before do
+          allow(deleted_scope).to receive(:destroy).and_return(false)
+        end
+
+        it do
+          expect do
+            expect do
+              subject
+            end.to raise_error(ActiveRecord::RecordNotDestroyed)
+          end.not_to change { AuditEvent.count }
+        end
+      end
+    end
+
+    context 'when new scope is being added' do
+      let(:new_environment_scope) { 'review' }
+      let(:params) do
+        {
+          scopes_attributes: [{ environment_scope: new_environment_scope, active: true }]
+        }
+      end
+
+      it 'creates audit event with new scope' do
+        expect(audit_event_message).to(
+          include("Created rule <strong>review</strong> and set it as <strong>active</strong>.")
+        )
+      end
+
+      context 'when scope can not be created' do
+        let(:new_environment_scope) { '' }
+
+        it { expect { subject }.not_to change { AuditEvent.count } }
+        it { expect(status).to eq(:error) }
+      end
+    end
+  end
+end
-- 
GitLab


From e199c7c39132624cd1b9c3a93923434a89a9a207 Mon Sep 17 00:00:00 2001
From: Vladimir Shushlin <v.shushlin@gmail.com>
Date: Wed, 6 Mar 2019 19:24:51 +0300
Subject: [PATCH 2/7] Add "feature_flag_audit" feature flag

* Add specs when feature_flag_audit is disabled
---
 ee/app/services/feature_flags/base_service.rb |   5 +
 .../services/feature_flags/create_service.rb  |   2 +-
 .../services/feature_flags/destroy_service.rb |   4 +-
 .../services/feature_flags/update_service.rb  |   4 +-
 .../user_creates_feature_flag_spec.rb         | 202 ++++++++++++------
 .../user_updates_feature_flag_spec.rb         | 162 ++++++++++----
 .../feature_flags/create_service_spec.rb      |  16 ++
 .../feature_flags/destroy_service_spec.rb     |  14 ++
 .../feature_flags/update_service_spec.rb      |  24 +++
 9 files changed, 317 insertions(+), 116 deletions(-)

diff --git a/ee/app/services/feature_flags/base_service.rb b/ee/app/services/feature_flags/base_service.rb
index fc8c03d67a47f4..b44701ef2b4a67 100644
--- a/ee/app/services/feature_flags/base_service.rb
+++ b/ee/app/services/feature_flags/base_service.rb
@@ -6,7 +6,12 @@ class BaseService < ::BaseService
 
     protected
 
+    def audit_enabled?
+      Feature.enabled?(:feature_flag_audit, project, default_enabled: true)
+    end
+
     def log_audit_event(feature_flag, message)
+      return unless audit_enabled?
       return unless message
 
       details =
diff --git a/ee/app/services/feature_flags/create_service.rb b/ee/app/services/feature_flags/create_service.rb
index d6a31eb3213cc0..a729ce573ed1ec 100644
--- a/ee/app/services/feature_flags/create_service.rb
+++ b/ee/app/services/feature_flags/create_service.rb
@@ -7,7 +7,7 @@ def execute
         feature_flag = project.operations_feature_flags.new(params)
 
         if feature_flag.save
-          log_audit_event(feature_flag, audit_message(feature_flag))
+          log_audit_event(feature_flag, audit_message(feature_flag)) if audit_enabled?
 
           success(feature_flag: feature_flag)
         else
diff --git a/ee/app/services/feature_flags/destroy_service.rb b/ee/app/services/feature_flags/destroy_service.rb
index d7c7c91cc0f161..192e8698205c2e 100644
--- a/ee/app/services/feature_flags/destroy_service.rb
+++ b/ee/app/services/feature_flags/destroy_service.rb
@@ -5,7 +5,9 @@ class DestroyService < FeatureFlags::BaseService
     def execute(feature_flag)
       ActiveRecord::Base.transaction do
         if feature_flag.destroy
-          log_audit_event(feature_flag, "Deleted feature flag <strong>#{feature_flag.name}</strong>.")
+          if audit_enabled?
+            log_audit_event(feature_flag, "Deleted feature flag <strong>#{feature_flag.name}</strong>.")
+          end
 
           success(feature_flag: feature_flag)
         else
diff --git a/ee/app/services/feature_flags/update_service.rb b/ee/app/services/feature_flags/update_service.rb
index 64afd91eedf37b..50a72814404c3c 100644
--- a/ee/app/services/feature_flags/update_service.rb
+++ b/ee/app/services/feature_flags/update_service.rb
@@ -11,10 +11,10 @@ def execute(feature_flag)
       ActiveRecord::Base.transaction do
         feature_flag.assign_attributes(params)
 
-        audit_log = audit_message(feature_flag)
+        audit_log = audit_message(feature_flag) if audit_enabled?
 
         if feature_flag.save
-          log_audit_event(feature_flag, audit_log)
+          log_audit_event(feature_flag, audit_log) if audit_enabled?
 
           success(feature_flag: feature_flag)
         else
diff --git a/ee/spec/features/projects/feature_flags/user_creates_feature_flag_spec.rb b/ee/spec/features/projects/feature_flags/user_creates_feature_flag_spec.rb
index 0a3f0a1824062d..b166affba44e2c 100644
--- a/ee/spec/features/projects/feature_flags/user_creates_feature_flag_spec.rb
+++ b/ee/spec/features/projects/feature_flags/user_creates_feature_flag_spec.rb
@@ -15,120 +15,182 @@
   end
 
   context 'when creates without changing scopes' do
-    before do
-      visit(new_project_feature_flag_path(project))
-      set_feature_flag_info('ci_live_trace', 'For live trace')
-      click_button 'Create feature flag'
-      expect(page).to have_current_path(project_feature_flags_path(project))
-    end
+    shared_examples 'succesfully creates feature flag' do
+      before do
+        visit(new_project_feature_flag_path(project))
+        set_feature_flag_info('ci_live_trace', 'For live trace')
+        click_button 'Create feature flag'
+        expect(page).to have_current_path(project_feature_flags_path(project))
+      end
 
-    it 'shows the created feature flag' do
-      within_feature_flag_row(1) do
-        expect(page.find('.feature-flag-name')).to have_content('ci_live_trace')
-        expect(page).to have_css('.js-feature-flag-status .badge-success')
+      it 'shows the created feature flag' do
+        within_feature_flag_row(1) do
+          expect(page.find('.feature-flag-name')).to have_content('ci_live_trace')
+          expect(page).to have_css('.js-feature-flag-status .badge-success')
 
-        within_feature_flag_scopes do
-          expect(page.find('.badge:nth-child(1)')).to have_content('*')
-          expect(page.find('.badge:nth-child(1)')['class']).to include('badge-active')
+          within_feature_flag_scopes do
+            expect(page.find('.badge:nth-child(1)')).to have_content('*')
+            expect(page.find('.badge:nth-child(1)')['class']).to include('badge-active')
+          end
         end
       end
     end
 
-    it 'records audit event' do
-      visit(project_audit_events_path(project))
+    context 'when feature flag audit enabled' do
+      include_examples 'succesfully creates feature flag'
+
+      it 'records audit event' do
+        visit(project_audit_events_path(project))
 
-      expect(page).to have_text("Created feature flag ci live trace with description \"For live trace\".")
+        expect(page).to have_text("Created feature flag ci live trace with description \"For live trace\".")
+      end
+    end
+
+    context 'when feature flag audit is disabled' do
+      before do
+        stub_feature_flags(feature_flag_audit: false)
+      end
+
+      include_examples 'succesfully creates feature flag'
+
+      it 'does not record audit event' do
+        visit(project_audit_events_path(project))
+
+        expect(page).to have_no_text("Created feature flag")
+      end
     end
   end
 
   context 'when creates with disabling the default scope' do
-    before do
-      visit(new_project_feature_flag_path(project))
-      set_feature_flag_info('ci_live_trace', 'For live trace')
+    shared_examples 'succesfully creates feature flag' do
+      before do
+        visit(new_project_feature_flag_path(project))
+        set_feature_flag_info('ci_live_trace', 'For live trace')
+
+        within_scope_row(1) do
+          within_status { find('.project-feature-toggle').click }
+        end
 
-      within_scope_row(1) do
-        within_status { find('.project-feature-toggle').click }
+        click_button 'Create feature flag'
       end
 
-      click_button 'Create feature flag'
+      it 'shows the created feature flag' do
+        within_feature_flag_row(1) do
+          expect(page.find('.feature-flag-name')).to have_content('ci_live_trace')
+          expect(page).to have_css('.js-feature-flag-status .badge-danger')
+
+          within_feature_flag_scopes do
+            expect(page.find('.badge:nth-child(1)')).to have_content('*')
+            expect(page.find('.badge:nth-child(1)')['class']).to include('badge-inactive')
+          end
+        end
+      end
     end
 
-    it 'shows the created feature flag' do
-      within_feature_flag_row(1) do
-        expect(page.find('.feature-flag-name')).to have_content('ci_live_trace')
-        expect(page).to have_css('.js-feature-flag-status .badge-danger')
+    context 'when feature flag audit enabled' do
+      include_examples 'succesfully creates feature flag'
+    end
 
-        within_feature_flag_scopes do
-          expect(page.find('.badge:nth-child(1)')).to have_content('*')
-          expect(page.find('.badge:nth-child(1)')['class']).to include('badge-inactive')
-        end
+    context 'when feature flag audit is disabled' do
+      before do
+        stub_feature_flags(feature_flag_audit: false)
       end
+
+      include_examples 'succesfully creates feature flag'
     end
   end
 
   context 'when creates with an additional scope' do
-    before do
-      visit(new_project_feature_flag_path(project))
-      set_feature_flag_info('mr_train', '')
-
-      within_scope_row(2) do
-        within_environment_spec do
-          find('.js-env-input').set("review/*")
-          find('.js-create-button').click
+    shared_examples 'succesfully creates feature flag' do
+      before do
+        visit(new_project_feature_flag_path(project))
+        set_feature_flag_info('mr_train', '')
+
+        within_scope_row(2) do
+          within_environment_spec do
+            find('.js-env-input').set("review/*")
+            find('.js-create-button').click
+          end
+        end
+
+        within_scope_row(2) do
+          within_status { find('.project-feature-toggle').click }
         end
-      end
 
-      within_scope_row(2) do
-        within_status { find('.project-feature-toggle').click }
+        click_button 'Create feature flag'
       end
 
-      click_button 'Create feature flag'
+      it 'shows the created feature flag' do
+        within_feature_flag_row(1) do
+          expect(page.find('.feature-flag-name')).to have_content('mr_train')
+          expect(page).to have_css('.js-feature-flag-status .badge-success')
+
+          within_feature_flag_scopes do
+            expect(page.find('.badge:nth-child(1)')).to have_content('*')
+            expect(page.find('.badge:nth-child(1)')['class']).to include('badge-active')
+            expect(page.find('.badge:nth-child(2)')).to have_content('review/*')
+            expect(page.find('.badge:nth-child(2)')['class']).to include('badge-active')
+          end
+        end
+      end
     end
 
-    it 'shows the created feature flag' do
-      within_feature_flag_row(1) do
-        expect(page.find('.feature-flag-name')).to have_content('mr_train')
-        expect(page).to have_css('.js-feature-flag-status .badge-success')
+    context 'when feature flag audit enabled' do
+      include_examples 'succesfully creates feature flag'
+    end
 
-        within_feature_flag_scopes do
-          expect(page.find('.badge:nth-child(1)')).to have_content('*')
-          expect(page.find('.badge:nth-child(1)')['class']).to include('badge-active')
-          expect(page.find('.badge:nth-child(2)')).to have_content('review/*')
-          expect(page.find('.badge:nth-child(2)')['class']).to include('badge-active')
-        end
+    context 'when feature flag audit is disabled' do
+      before do
+        stub_feature_flags(feature_flag_audit: false)
       end
+
+      include_examples 'succesfully creates feature flag'
     end
   end
 
   context 'when searches an environment name for scope creation' do
     let!(:environment) { create(:environment, name: 'production', project: project) }
 
-    before do
-      visit(new_project_feature_flag_path(project))
-      set_feature_flag_info('mr_train', '')
+    shared_examples 'succesfully creates feature flag' do
+      before do
+        visit(new_project_feature_flag_path(project))
+        set_feature_flag_info('mr_train', '')
 
-      within_scope_row(2) do
-        within_environment_spec do
-          find('.js-env-input').set('prod')
-          click_button 'production'
+        within_scope_row(2) do
+          within_environment_spec do
+            find('.js-env-input').set('prod')
+            click_button 'production'
+          end
         end
+
+        click_button 'Create feature flag'
       end
 
-      click_button 'Create feature flag'
+      it 'shows the created feature flag' do
+        within_feature_flag_row(1) do
+          expect(page.find('.feature-flag-name')).to have_content('mr_train')
+          expect(page).to have_css('.js-feature-flag-status .badge-success')
+
+          within_feature_flag_scopes do
+            expect(page.find('.badge:nth-child(1)')).to have_content('*')
+            expect(page.find('.badge:nth-child(1)')['class']).to include('badge-active')
+            expect(page.find('.badge:nth-child(2)')).to have_content('production')
+            expect(page.find('.badge:nth-child(2)')['class']).to include('badge-inactive')
+          end
+        end
+      end
     end
 
-    it 'shows the created feature flag' do
-      within_feature_flag_row(1) do
-        expect(page.find('.feature-flag-name')).to have_content('mr_train')
-        expect(page).to have_css('.js-feature-flag-status .badge-success')
+    context 'when feature flag audit enabled' do
+      include_examples 'succesfully creates feature flag'
+    end
 
-        within_feature_flag_scopes do
-          expect(page.find('.badge:nth-child(1)')).to have_content('*')
-          expect(page.find('.badge:nth-child(1)')['class']).to include('badge-active')
-          expect(page.find('.badge:nth-child(2)')).to have_content('production')
-          expect(page.find('.badge:nth-child(2)')['class']).to include('badge-inactive')
-        end
+    context 'when feature flag audit is disabled' do
+      before do
+        stub_feature_flags(feature_flag_audit: false)
       end
+
+      include_examples 'succesfully creates feature flag'
     end
   end
 
diff --git a/ee/spec/features/projects/feature_flags/user_updates_feature_flag_spec.rb b/ee/spec/features/projects/feature_flags/user_updates_feature_flag_spec.rb
index 5d49da1519dc67..a9d908fc6f759e 100644
--- a/ee/spec/features/projects/feature_flags/user_updates_feature_flag_spec.rb
+++ b/ee/spec/features/projects/feature_flags/user_updates_feature_flag_spec.rb
@@ -37,75 +37,153 @@
   end
 
   context 'when user updates a status of a scope' do
-    before do
-      within_scope_row(2) do
-        within_status { find('.project-feature-toggle').click }
+    shared_examples 'succesfully updates feature flag' do
+      before do
+        within_scope_row(2) do
+          within_status { find('.project-feature-toggle').click }
+        end
+
+        click_button 'Save changes'
+        expect(page).to have_current_path(project_feature_flags_path(project))
       end
 
-      click_button 'Save changes'
-      expect(page).to have_current_path(project_feature_flags_path(project))
+      it 'shows the updated feature flag' do
+        within_feature_flag_row(1) do
+          expect(page.find('.feature-flag-name')).to have_content('ci_live_trace')
+          expect(page).to have_css('.js-feature-flag-status .badge-danger')
+
+          within_feature_flag_scopes do
+            expect(page.find('.badge:nth-child(1)')).to have_content('*')
+            expect(page.find('.badge:nth-child(1)')['class']).to include('badge-inactive')
+            expect(page.find('.badge:nth-child(2)')).to have_content('review/*')
+            expect(page.find('.badge:nth-child(2)')['class']).to include('badge-inactive')
+          end
+        end
+      end
     end
 
-    it 'shows the updated feature flag' do
-      within_feature_flag_row(1) do
-        expect(page.find('.feature-flag-name')).to have_content('ci_live_trace')
-        expect(page).to have_css('.js-feature-flag-status .badge-danger')
+    context 'when feature flag audit enabled' do
+      include_examples 'succesfully updates feature flag'
 
-        within_feature_flag_scopes do
-          expect(page.find('.badge:nth-child(1)')).to have_content('*')
-          expect(page.find('.badge:nth-child(1)')['class']).to include('badge-inactive')
-          expect(page.find('.badge:nth-child(2)')).to have_content('review/*')
-          expect(page.find('.badge:nth-child(2)')['class']).to include('badge-inactive')
-        end
+      it 'records audit event' do
+        visit(project_audit_events_path(project))
+
+        expect(page).to(
+          have_text("Updated feature flag ci live trace. Updated rule review/* active state from true to false.")
+        )
       end
     end
 
-    it 'records audit event' do
-      visit(project_audit_events_path(project))
+    context 'when feature flag audit is disabled' do
+      before do
+        stub_feature_flags(feature_flag_audit: false)
+      end
+
+      include_examples 'succesfully updates feature flag'
 
-      expect(page).to(
-        have_text("Updated feature flag ci live trace. Updated rule review/* active state from true to false.")
-      )
+      it 'does not record audit event' do
+        visit(project_audit_events_path(project))
+
+        expect(page).to have_no_text("Updated feature flag")
+      end
     end
   end
 
   context 'when user adds a new scope' do
-    before do
-      within_scope_row(3) do
-        within_environment_spec do
-          find('.js-env-input').set('production')
-          find('.js-create-button').click
+    shared_examples 'succesfully updates feature flag' do
+      before do
+        within_scope_row(3) do
+          within_environment_spec do
+            find('.js-env-input').set('production')
+            find('.js-create-button').click
+          end
         end
+
+        click_button 'Save changes'
+        expect(page).to have_current_path(project_feature_flags_path(project))
       end
 
-      click_button 'Save changes'
+      it 'shows the newly created scope' do
+        within_feature_flag_row(1) do
+          within_feature_flag_scopes do
+            expect(page.find('.badge:nth-child(3)')).to have_content('production')
+            expect(page.find('.badge:nth-child(3)')['class']).to include('badge-inactive')
+          end
+        end
+      end
     end
 
-    it 'shows the newly created scope' do
-      within_feature_flag_row(1) do
-        within_feature_flag_scopes do
-          expect(page.find('.badge:nth-child(3)')).to have_content('production')
-          expect(page.find('.badge:nth-child(3)')['class']).to include('badge-inactive')
-        end
+    context 'when feature flag audit enabled' do
+      include_examples 'succesfully updates feature flag'
+
+      it 'records audit event' do
+        visit(project_audit_events_path(project))
+
+        expect(page).to(
+          have_text("Updated feature flag ci live trace")
+        )
+      end
+    end
+
+    context 'when feature flag audit is disabled' do
+      before do
+        stub_feature_flags(feature_flag_audit: false)
+      end
+
+      include_examples 'succesfully updates feature flag'
+
+      it 'does not record audit event' do
+        visit(project_audit_events_path(project))
+
+        expect(page).to have_no_text("Updated feature flag")
       end
     end
   end
 
   context 'when user deletes a scope' do
-    before do
-      within_scope_row(2) do
-        within_delete { find('.js-delete-scope').click }
+    shared_examples 'succesfully updates feature flag' do
+      before do
+        within_scope_row(2) do
+          within_delete { find('.js-delete-scope').click }
+        end
+
+        click_button 'Save changes'
+        expect(page).to have_current_path(project_feature_flags_path(project))
       end
 
-      click_button 'Save changes'
+      it 'shows the updated feature flag' do
+        within_feature_flag_row(1) do
+          within_feature_flag_scopes do
+            expect(page).to have_css('.badge:nth-child(1)')
+            expect(page).not_to have_css('.badge:nth-child(2)')
+          end
+        end
+      end
     end
 
-    it 'shows the updated feature flag' do
-      within_feature_flag_row(1) do
-        within_feature_flag_scopes do
-          expect(page).to have_css('.badge:nth-child(1)')
-          expect(page).not_to have_css('.badge:nth-child(2)')
-        end
+    context 'when feature flag audit enabled' do
+      include_examples 'succesfully updates feature flag'
+
+      it 'records audit event' do
+        visit(project_audit_events_path(project))
+
+        expect(page).to(
+          have_text("Updated feature flag ci live trace")
+        )
+      end
+    end
+
+    context 'when feature flag audit is disabled' do
+      before do
+        stub_feature_flags(feature_flag_audit: false)
+      end
+
+      include_examples 'succesfully updates feature flag'
+
+      it 'does not record audit event' do
+        visit(project_audit_events_path(project))
+
+        expect(page).to have_no_text("Updated feature flag")
       end
     end
   end
diff --git a/ee/spec/services/feature_flags/create_service_spec.rb b/ee/spec/services/feature_flags/create_service_spec.rb
index 8328389d2d1747..eae794e574e57e 100644
--- a/ee/spec/services/feature_flags/create_service_spec.rb
+++ b/ee/spec/services/feature_flags/create_service_spec.rb
@@ -46,6 +46,22 @@
         expect { subject }.to change { AuditEvent.count }.by(1)
         expect(AuditEvent.last.present.action).to eq(expected_message)
       end
+
+      context 'when feature flag audit is disabled' do
+        before do
+          stub_feature_flags(feature_flag_audit: false)
+        end
+
+        it { expect(status).to eq(:success) }
+
+        it 'creates feature flag' do
+          expect { subject }.to change { Operations::FeatureFlag.count }.by(1)
+        end
+
+        it 'does not create audit log' do
+          expect { subject }.not_to change { AuditEvent.count }
+        end
+      end
     end
   end
 end
diff --git a/ee/spec/services/feature_flags/destroy_service_spec.rb b/ee/spec/services/feature_flags/destroy_service_spec.rb
index 7bcf6093bf726b..7327b5bf7380ec 100644
--- a/ee/spec/services/feature_flags/destroy_service_spec.rb
+++ b/ee/spec/services/feature_flags/destroy_service_spec.rb
@@ -21,6 +21,20 @@
     it { expect { subject }.to change { AuditEvent.count }.by(1) }
     it { expect(audit_event_message).to eq("Deleted feature flag <strong>#{feature_flag.name.tr('_', ' ')}</strong>.") }
 
+    context 'when feature flag audit is disabled' do
+      before do
+        stub_feature_flags(feature_flag_audit: false)
+      end
+
+      it 'works successfully' do
+        expect(status).to eq(:success)
+      end
+
+      it 'does not create audit event' do
+        expect { subject }.not_to change { AuditEvent.count }
+      end
+    end
+
     context 'when feature flag can not be destroyed' do
       before do
         allow(feature_flag).to receive(:destroy).and_return(false)
diff --git a/ee/spec/services/feature_flags/update_service_spec.rb b/ee/spec/services/feature_flags/update_service_spec.rb
index 9d34d3535cc273..1a93a725a3eca3 100644
--- a/ee/spec/services/feature_flags/update_service_spec.rb
+++ b/ee/spec/services/feature_flags/update_service_spec.rb
@@ -7,6 +7,20 @@
   let(:user) { create(:user) }
   let(:feature_flag) { create(:operations_feature_flag) }
 
+  shared_examples 'when feature flag audit is disabled' do
+    before do
+      stub_feature_flags(feature_flag_audit: false)
+    end
+
+    it 'works successfully' do
+      expect(status).to eq(:success)
+    end
+
+    it 'does not create audit event' do
+      expect { subject }.not_to change { AuditEvent.count }
+    end
+  end
+
   describe '#execute' do
     subject { described_class.new(project, user, params).execute(feature_flag) }
     let(:status) { subject[:status] }
@@ -56,6 +70,8 @@
                   " to <strong>\"new description\"</strong>.")
         )
       end
+
+      it_behaves_like 'when feature flag audit is disabled'
     end
 
     context 'when active state is changed' do
@@ -71,6 +87,8 @@
                   "from <strong>true</strong> to <strong>false</strong>.")
         )
       end
+
+      it_behaves_like 'when feature flag audit is disabled'
     end
 
     context 'when scope is renamed' do
@@ -89,6 +107,8 @@
         )
       end
 
+      it_behaves_like 'when feature flag audit is disabled'
+
       context 'when scope can not be updated' do
         let(:new_environment_scope) { '' }
 
@@ -107,6 +127,8 @@
 
       it { expect(audit_event_message).to include("Deleted rule <strong>review</strong>.") }
 
+      it_behaves_like 'when feature flag audit is disabled'
+
       context 'when scope can not be deleted' do
         before do
           allow(deleted_scope).to receive(:destroy).and_return(false)
@@ -136,6 +158,8 @@
         )
       end
 
+      it_behaves_like 'when feature flag audit is disabled'
+
       context 'when scope can not be created' do
         let(:new_environment_scope) { '' }
 
-- 
GitLab


From 0e7cd6b119abf69c63bfc6ba82c40c90efcc8b0d Mon Sep 17 00:00:00 2001
From: Vladimir Shushlin <v.shushlin@gmail.com>
Date: Thu, 7 Mar 2019 09:46:57 +0300
Subject: [PATCH 3/7] Remove redundant guard statement

---
 ee/app/services/feature_flags/base_service.rb   | 1 -
 ee/app/services/feature_flags/update_service.rb | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/ee/app/services/feature_flags/base_service.rb b/ee/app/services/feature_flags/base_service.rb
index b44701ef2b4a67..e112052e1069f5 100644
--- a/ee/app/services/feature_flags/base_service.rb
+++ b/ee/app/services/feature_flags/base_service.rb
@@ -12,7 +12,6 @@ def audit_enabled?
 
     def log_audit_event(feature_flag, message)
       return unless audit_enabled?
-      return unless message
 
       details =
         {
diff --git a/ee/app/services/feature_flags/update_service.rb b/ee/app/services/feature_flags/update_service.rb
index 50a72814404c3c..7c7aaa8cdcff3c 100644
--- a/ee/app/services/feature_flags/update_service.rb
+++ b/ee/app/services/feature_flags/update_service.rb
@@ -14,7 +14,7 @@ def execute(feature_flag)
         audit_log = audit_message(feature_flag) if audit_enabled?
 
         if feature_flag.save
-          log_audit_event(feature_flag, audit_log) if audit_enabled?
+          log_audit_event(feature_flag, audit_log) unless audit_log.blank?
 
           success(feature_flag: feature_flag)
         else
-- 
GitLab


From 16c298a3cc719521a66fb02cbbba44bed6290681 Mon Sep 17 00:00:00 2001
From: Vladimir Shushlin <v.shushlin@gmail.com>
Date: Thu, 7 Mar 2019 11:24:55 +0300
Subject: [PATCH 4/7] Refactor FeatureFlags::UpdateService spec

---
 .../feature_flags/update_service_spec.rb      | 132 ++++++++++++------
 1 file changed, 87 insertions(+), 45 deletions(-)

diff --git a/ee/spec/services/feature_flags/update_service_spec.rb b/ee/spec/services/feature_flags/update_service_spec.rb
index 1a93a725a3eca3..fcb951fcb0bc1e 100644
--- a/ee/spec/services/feature_flags/update_service_spec.rb
+++ b/ee/spec/services/feature_flags/update_service_spec.rb
@@ -7,37 +7,21 @@
   let(:user) { create(:user) }
   let(:feature_flag) { create(:operations_feature_flag) }
 
-  shared_examples 'when feature flag audit is disabled' do
-    before do
-      stub_feature_flags(feature_flag_audit: false)
-    end
-
-    it 'works successfully' do
-      expect(status).to eq(:success)
-    end
-
-    it 'does not create audit event' do
-      expect { subject }.not_to change { AuditEvent.count }
-    end
-  end
-
   describe '#execute' do
     subject { described_class.new(project, user, params).execute(feature_flag) }
-    let(:status) { subject[:status] }
-    let(:returned_feature_flag) { subject[:feature_flag] }
-    let(:errors) { subject[:message] }
     let(:params) { { name: 'new_name' } }
     let(:audit_event_message) do
-      subject
       AuditEvent.last.present.action
     end
 
-    it { expect(status).to eq(:success) }
-    it { expect(returned_feature_flag).to be_valid }
-    it { expect { subject }.to change { AuditEvent.count }.by(1) }
+    it 'returns success status' do
+      expect(subject[:status]).to eq(:success)
+    end
+
     it 'creates audit event with correct message' do
       name_was = feature_flag.name
 
+      expect { subject }.to change { AuditEvent.count }.by(1)
       expect(audit_event_message).to(
         eq("Updated feature flag <strong>new name</strong>. "\
            "Updated name from <strong>\"#{name_was.tr('_', ' ')}\"</strong> "\
@@ -45,33 +29,64 @@
       )
     end
 
+    shared_examples 'disabled feature flag audit' do
+      context 'when feature flag audit is disabled' do
+        before do
+          stub_feature_flags(feature_flag_audit: false)
+        end
+
+        it 'returns success status' do
+          expect(subject[:status]).to eq(:success)
+        end
+
+        it 'does not create audit event' do
+          expect { subject }.not_to change { AuditEvent.count }
+        end
+      end
+    end
+
+    include_examples 'disabled feature flag audit'
+
     context 'with invalid params' do
       let(:params) { { name: nil } }
 
-      it { expect(status).to eq(:error) }
-      it { expect(errors).to include("Name can't be blank") }
-      it { expect { subject }.not_to change { AuditEvent.count } }
+      it 'returns error status' do
+        expect(subject[:status]).to eq(:error)
+      end
+
+      it 'returns error messages' do
+        expect(subject[:message]).to include("Name can't be blank")
+      end
+
+      it 'does not create audit event' do
+        expect { subject }.not_to change { AuditEvent.count }
+      end
     end
 
     context 'when nothing is changed' do
       let(:params) { {} }
 
-      it { expect(status).to eq(:success) }
-      it { expect(returned_feature_flag).to be_valid }
-      it { expect { subject }.not_to change { AuditEvent.count } }
+      it 'returns success status' do
+        expect(subject[:status]).to eq(:success)
+      end
+
+      it 'does not create audit event' do
+        expect { subject }.not_to change { AuditEvent.count }
+      end
     end
 
     context 'description is being changed' do
       let(:params) { { description: 'new description' } }
 
       it 'creates audit event with changed description' do
+        expect { subject }.to change { AuditEvent.count }.by(1)
         expect(audit_event_message).to(
           include("Updated description from <strong>\"\"</strong>"\
                   " to <strong>\"new description\"</strong>.")
         )
       end
 
-      it_behaves_like 'when feature flag audit is disabled'
+      include_examples 'disabled feature flag audit'
     end
 
     context 'when active state is changed' do
@@ -82,38 +97,52 @@
       end
 
       it 'creates audit event about changing active stae' do
+        expect { subject }.to change { AuditEvent.count }.by(1)
         expect(audit_event_message).to(
           include("Updated rule <strong>*</strong> active state "\
                   "from <strong>true</strong> to <strong>false</strong>.")
         )
       end
 
-      it_behaves_like 'when feature flag audit is disabled'
+      include_examples 'disabled feature flag audit'
     end
 
     context 'when scope is renamed' do
       let(:changed_scope) { feature_flag.scopes.create!(environment_scope: 'review', active: true) }
-      let(:new_environment_scope) { 'staging' }
       let(:params) do
         {
-          scopes_attributes: [{ id: changed_scope.id, environment_scope: new_environment_scope }]
+          scopes_attributes: [{ id: changed_scope.id, environment_scope: 'staging' }]
         }
       end
 
       it 'creates audit event with changed name' do
+        expect { subject }.to change { AuditEvent.count }.by(1)
         expect(audit_event_message).to(
           include("Updated rule <strong>staging</strong> environment scope "\
                   "from <strong>review</strong> to <strong>staging</strong>.")
         )
       end
 
-      it_behaves_like 'when feature flag audit is disabled'
+      include_examples 'disabled feature flag audit'
 
       context 'when scope can not be updated' do
-        let(:new_environment_scope) { '' }
+        let(:params) do
+          {
+            scopes_attributes: [{ id: changed_scope.id, environment_scope: '' }]
+          }
+        end
+
+        it 'returns error status' do
+          expect(subject[:status]).to eq(:error)
+        end
 
-        it { expect { subject }.not_to change { AuditEvent.count } }
-        it { expect(status).to eq(:error) }
+        it 'returns error messages' do
+          expect(subject[:message]).to include("Scopes environment scope can't be blank")
+        end
+
+        it 'does not create audit event' do
+          expect { subject }.not_to change { AuditEvent.count }
+        end
       end
     end
 
@@ -125,21 +154,24 @@
         }
       end
 
-      it { expect(audit_event_message).to include("Deleted rule <strong>review</strong>.") }
+      it 'creates audit event with deleted scope' do
+        expect { subject }.to change { AuditEvent.count }.by(1)
+        expect(audit_event_message).to include("Deleted rule <strong>review</strong>.")
+      end
 
-      it_behaves_like 'when feature flag audit is disabled'
+      include_examples 'disabled feature flag audit'
 
       context 'when scope can not be deleted' do
+        RSpec::Matchers.define_negated_matcher :not_change, :change
+
         before do
           allow(deleted_scope).to receive(:destroy).and_return(false)
         end
 
-        it do
+        it 'does not create audit event' do
           expect do
-            expect do
-              subject
-            end.to raise_error(ActiveRecord::RecordNotDestroyed)
-          end.not_to change { AuditEvent.count }
+            subject
+          end.to not_change { AuditEvent.count }.and raise_error(ActiveRecord::RecordNotDestroyed)
         end
       end
     end
@@ -153,18 +185,28 @@
       end
 
       it 'creates audit event with new scope' do
+        subject
         expect(audit_event_message).to(
           include("Created rule <strong>review</strong> and set it as <strong>active</strong>.")
         )
       end
 
-      it_behaves_like 'when feature flag audit is disabled'
+      include_examples 'disabled feature flag audit'
 
       context 'when scope can not be created' do
         let(:new_environment_scope) { '' }
 
-        it { expect { subject }.not_to change { AuditEvent.count } }
-        it { expect(status).to eq(:error) }
+        it 'returns error status' do
+          expect(subject[:status]).to eq(:error)
+        end
+
+        it 'returns error messages' do
+          expect(subject[:message]).to include("Scopes environment scope can't be blank")
+        end
+
+        it 'does not create audit event' do
+          expect { subject }.not_to change { AuditEvent.count }
+        end
       end
     end
   end
-- 
GitLab


From f9fe9ad282231f9f6ae4a3be99a8d6f39b544003 Mon Sep 17 00:00:00 2001
From: Vladimir Shushlin <v.shushlin@gmail.com>
Date: Thu, 7 Mar 2019 14:38:58 +0300
Subject: [PATCH 5/7] Refactor audit_enabled check for feature flags

---
 ee/app/services/feature_flags/base_service.rb    |  8 ++++++--
 ee/app/services/feature_flags/create_service.rb  |  2 +-
 ee/app/services/feature_flags/destroy_service.rb | 10 +++++++---
 ee/app/services/feature_flags/update_service.rb  |  4 ++--
 4 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/ee/app/services/feature_flags/base_service.rb b/ee/app/services/feature_flags/base_service.rb
index e112052e1069f5..092eaf54b8d051 100644
--- a/ee/app/services/feature_flags/base_service.rb
+++ b/ee/app/services/feature_flags/base_service.rb
@@ -10,9 +10,13 @@ def audit_enabled?
       Feature.enabled?(:feature_flag_audit, project, default_enabled: true)
     end
 
-    def log_audit_event(feature_flag, message)
+    def build_audit_event(feature_flag)
       return unless audit_enabled?
 
+      message = audit_message(feature_flag)
+
+      return unless message
+
       details =
         {
           custom_message: message,
@@ -25,7 +29,7 @@ def log_audit_event(feature_flag, message)
         current_user,
         feature_flag.project,
         details
-      ).security_event
+      )
     end
 
     def created_scope_message(scope)
diff --git a/ee/app/services/feature_flags/create_service.rb b/ee/app/services/feature_flags/create_service.rb
index a729ce573ed1ec..927367c9029ca8 100644
--- a/ee/app/services/feature_flags/create_service.rb
+++ b/ee/app/services/feature_flags/create_service.rb
@@ -7,7 +7,7 @@ def execute
         feature_flag = project.operations_feature_flags.new(params)
 
         if feature_flag.save
-          log_audit_event(feature_flag, audit_message(feature_flag)) if audit_enabled?
+          build_audit_event(feature_flag)&.security_event
 
           success(feature_flag: feature_flag)
         else
diff --git a/ee/app/services/feature_flags/destroy_service.rb b/ee/app/services/feature_flags/destroy_service.rb
index 192e8698205c2e..4dd77287a7e14d 100644
--- a/ee/app/services/feature_flags/destroy_service.rb
+++ b/ee/app/services/feature_flags/destroy_service.rb
@@ -5,9 +5,7 @@ class DestroyService < FeatureFlags::BaseService
     def execute(feature_flag)
       ActiveRecord::Base.transaction do
         if feature_flag.destroy
-          if audit_enabled?
-            log_audit_event(feature_flag, "Deleted feature flag <strong>#{feature_flag.name}</strong>.")
-          end
+          build_audit_event(feature_flag)&.security_event
 
           success(feature_flag: feature_flag)
         else
@@ -15,5 +13,11 @@ def execute(feature_flag)
         end
       end
     end
+
+    private
+
+    def audit_message(feature_flag)
+      "Deleted feature flag <strong>#{feature_flag.name}</strong>."
+    end
   end
 end
diff --git a/ee/app/services/feature_flags/update_service.rb b/ee/app/services/feature_flags/update_service.rb
index 7c7aaa8cdcff3c..2aed80ed18a04e 100644
--- a/ee/app/services/feature_flags/update_service.rb
+++ b/ee/app/services/feature_flags/update_service.rb
@@ -11,10 +11,10 @@ def execute(feature_flag)
       ActiveRecord::Base.transaction do
         feature_flag.assign_attributes(params)
 
-        audit_log = audit_message(feature_flag) if audit_enabled?
+        audit_event = build_audit_event(feature_flag)
 
         if feature_flag.save
-          log_audit_event(feature_flag, audit_log) unless audit_log.blank?
+          audit_event&.security_event
 
           success(feature_flag: feature_flag)
         else
-- 
GitLab


From 9ccd3b6bd1bf318c9b3a890323293e262ae54e4a Mon Sep 17 00:00:00 2001
From: Vladimir Shushlin <v.shushlin@gmail.com>
Date: Thu, 7 Mar 2019 15:08:34 +0300
Subject: [PATCH 6/7] Refactor feature flags create and destroy service spec

---
 .../feature_flags/create_service_spec.rb      | 23 +++++++++----
 .../feature_flags/destroy_service_spec.rb     | 32 ++++++++++++-------
 2 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/ee/spec/services/feature_flags/create_service_spec.rb b/ee/spec/services/feature_flags/create_service_spec.rb
index eae794e574e57e..16c4554527f76c 100644
--- a/ee/spec/services/feature_flags/create_service_spec.rb
+++ b/ee/spec/services/feature_flags/create_service_spec.rb
@@ -10,15 +10,22 @@
     subject do
       described_class.new(project, user, params).execute
     end
-    let(:status) { subject[:status] }
     let(:feature_flag) { subject[:feature_flag] }
-    let(:errors) { subject[:message] }
 
     context 'when feature flag can not be created' do
       let(:params) { {} }
-      it { expect(status).to eq(:error) }
-      it { expect(errors).to include("Name can't be blank") }
-      it { expect { subject }.not_to change { AuditEvent.count } }
+
+      it 'returns status error' do
+        expect(subject[:status]).to eq(:error)
+      end
+
+      it 'returns validation errors' do
+        expect(subject[:message]).to include("Name can't be blank")
+      end
+
+      it 'does not create audit log' do
+        expect { subject }.not_to change { AuditEvent.count }
+      end
     end
 
     context 'when feature flag is saved correctly' do
@@ -31,7 +38,9 @@
         }
       end
 
-      it { expect(status).to eq(:success) }
+      it 'returns status success' do
+        expect(subject[:status]).to eq(:success)
+      end
 
       it 'creates feature flag' do
         expect { subject }.to change { Operations::FeatureFlag.count }.by(1)
@@ -52,7 +61,7 @@
           stub_feature_flags(feature_flag_audit: false)
         end
 
-        it { expect(status).to eq(:success) }
+        it { expect(subject[:status]).to eq(:success) }
 
         it 'creates feature flag' do
           expect { subject }.to change { Operations::FeatureFlag.count }.by(1)
diff --git a/ee/spec/services/feature_flags/destroy_service_spec.rb b/ee/spec/services/feature_flags/destroy_service_spec.rb
index 7327b5bf7380ec..302a91b57c4f1d 100644
--- a/ee/spec/services/feature_flags/destroy_service_spec.rb
+++ b/ee/spec/services/feature_flags/destroy_service_spec.rb
@@ -9,17 +9,20 @@
 
   describe '#execute' do
     subject { described_class.new(project, user).execute(feature_flag) }
-    let(:status) { subject[:status] }
-    let(:audit_event) do
-      subject
-      AuditEvent.last
+    let(:audit_event_message) { AuditEvent.last.present.action }
+
+    it 'returns status success' do
+      expect(subject[:status]).to eq(:success)
+    end
+
+    it 'destroys feature flag' do
+      expect { subject }.to change { Operations::FeatureFlag.count }.by(-1)
     end
-    let(:audit_event_message) { audit_event.present.action }
 
-    it { expect(status).to eq(:success) }
-    it { expect { subject }.to change { Operations::FeatureFlag.count }.by(-1) }
-    it { expect { subject }.to change { AuditEvent.count }.by(1) }
-    it { expect(audit_event_message).to eq("Deleted feature flag <strong>#{feature_flag.name.tr('_', ' ')}</strong>.") }
+    it 'creates audit log' do
+      expect { subject }.to change { AuditEvent.count }.by(1)
+      expect(audit_event_message).to eq("Deleted feature flag <strong>#{feature_flag.name.tr('_', ' ')}</strong>.")
+    end
 
     context 'when feature flag audit is disabled' do
       before do
@@ -27,7 +30,7 @@
       end
 
       it 'works successfully' do
-        expect(status).to eq(:success)
+        expect(subject[:status]).to eq(:success)
       end
 
       it 'does not create audit event' do
@@ -40,8 +43,13 @@
         allow(feature_flag).to receive(:destroy).and_return(false)
       end
 
-      it { expect(status).to eq(:error) }
-      it { expect { subject }.not_to change { AuditEvent.count } }
+      it 'returns status error' do
+        expect(subject[:status]).to eq(:error)
+      end
+
+      it 'does not create audit log' do
+        expect { subject }.not_to change { AuditEvent.count }
+      end
     end
   end
 end
-- 
GitLab


From 20c46eeaf66958899af7fb8695d79d99cc5cfbc1 Mon Sep 17 00:00:00 2001
From: Vladimir Shushlin <v.shushlin@gmail.com>
Date: Fri, 8 Mar 2019 01:45:43 +0300
Subject: [PATCH 7/7] Explicitly save audit events

---
 ee/app/services/feature_flags/base_service.rb    | 10 ++++++++--
 ee/app/services/feature_flags/create_service.rb  |  2 +-
 ee/app/services/feature_flags/destroy_service.rb |  2 +-
 ee/app/services/feature_flags/update_service.rb  |  4 ++--
 4 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/ee/app/services/feature_flags/base_service.rb b/ee/app/services/feature_flags/base_service.rb
index 092eaf54b8d051..b5a05900f6e646 100644
--- a/ee/app/services/feature_flags/base_service.rb
+++ b/ee/app/services/feature_flags/base_service.rb
@@ -10,12 +10,12 @@ def audit_enabled?
       Feature.enabled?(:feature_flag_audit, project, default_enabled: true)
     end
 
-    def build_audit_event(feature_flag)
+    def audit_event(feature_flag)
       return unless audit_enabled?
 
       message = audit_message(feature_flag)
 
-      return unless message
+      return if message.blank?
 
       details =
         {
@@ -32,6 +32,12 @@ def build_audit_event(feature_flag)
       )
     end
 
+    def save_audit_event(audit_event)
+      return unless audit_event # feature_flag_audit is disabled or audit_message is blank
+
+      audit_event.security_event
+    end
+
     def created_scope_message(scope)
       "Created rule <strong>#{scope.environment_scope}</strong> "\
       "and set it as <strong>#{scope.active ? "active" : "inactive"}</strong>."
diff --git a/ee/app/services/feature_flags/create_service.rb b/ee/app/services/feature_flags/create_service.rb
index 927367c9029ca8..bcc0c097276801 100644
--- a/ee/app/services/feature_flags/create_service.rb
+++ b/ee/app/services/feature_flags/create_service.rb
@@ -7,7 +7,7 @@ def execute
         feature_flag = project.operations_feature_flags.new(params)
 
         if feature_flag.save
-          build_audit_event(feature_flag)&.security_event
+          save_audit_event(audit_event(feature_flag))
 
           success(feature_flag: feature_flag)
         else
diff --git a/ee/app/services/feature_flags/destroy_service.rb b/ee/app/services/feature_flags/destroy_service.rb
index 4dd77287a7e14d..c80d869787cdeb 100644
--- a/ee/app/services/feature_flags/destroy_service.rb
+++ b/ee/app/services/feature_flags/destroy_service.rb
@@ -5,7 +5,7 @@ class DestroyService < FeatureFlags::BaseService
     def execute(feature_flag)
       ActiveRecord::Base.transaction do
         if feature_flag.destroy
-          build_audit_event(feature_flag)&.security_event
+          save_audit_event(audit_event(feature_flag))
 
           success(feature_flag: feature_flag)
         else
diff --git a/ee/app/services/feature_flags/update_service.rb b/ee/app/services/feature_flags/update_service.rb
index 2aed80ed18a04e..a9612a7f4a5a76 100644
--- a/ee/app/services/feature_flags/update_service.rb
+++ b/ee/app/services/feature_flags/update_service.rb
@@ -11,10 +11,10 @@ def execute(feature_flag)
       ActiveRecord::Base.transaction do
         feature_flag.assign_attributes(params)
 
-        audit_event = build_audit_event(feature_flag)
+        audit_event = audit_event(feature_flag)
 
         if feature_flag.save
-          audit_event&.security_event
+          save_audit_event(audit_event)
 
           success(feature_flag: feature_flag)
         else
-- 
GitLab