Merge branch '10359-feature-flag-keep-underscore' into 'master'

Resolve "Feature flag audit log substitute `_` to ` `"

Closes #10359

See merge request gitlab-org/gitlab-ee!14621

ee/app/helpers/audit_events_helper.rb

@@ -2,8 +2,7 @@
 
 module AuditEventsHelper
   def human_text(details)
-    # replace '_' with " " to achive identical behavior with Audit::Details
-    return details[:custom_message].tr('_', ' ') if details[:custom_message]
+    return details[:custom_message] if details[:custom_message]
 
     details.map { |key, value| select_keys(key, value) }.join(" ").humanize
   end

ee/changelogs/unreleased/10359-feature-flag-keep-underscore.yml

+---
+title: Fix displaying feature flag names in the audit log
+merge_request: 14621
+author:
+type: fixed

ee/lib/audit/details.rb

@@ -24,19 +24,18 @@ module Audit
 
     def action_text
       action = @details.slice(*ACTIONS)
-      value = @details.values.first.tr('_', ' ')
 
       case action.keys.first
       when :add
-        "Added #{value}#{@details[:as] ? " as #{@details[:as]}" : ''}"
+        "Added #{target_name}#{@details[:as] ? " as #{@details[:as]}" : ''}"
       when :remove
-        "Removed #{value}"
+        "Removed #{target_name}"
       when :failed_login
-        "Failed to login with #{Gitlab::Auth::OAuth::Provider.label_for(value).upcase} authentication"
+        "Failed to login with #{oauth_label} authentication"
       when :custom_message
-        value
+        detail_value
       else
-        text_for_change(value)
+        text_for_change(target_name)
       end
     end
 
@@ -48,5 +47,21 @@ module Audit
 
       changed.join(' ')
     end
+
+    def target_name
+      @details[:target_type] == 'Operations::FeatureFlag' ? detail_value : target_name_with_space
+    end
+
+    def target_name_with_space
+      detail_value.tr('_', ' ')
+    end
+
+    def detail_value
+      @details.values.first
+    end
+
+    def oauth_label
+      Gitlab::Auth::OAuth::Provider.label_for(detail_value).upcase
+    end
   end
 end

ee/spec/features/admin/admin_audit_logs_spec.rb

@@ -103,10 +103,6 @@ describe 'Admin::AuditLogs', :js do
 
         expect(page).to have_content('Removed user access')
       end
-
-      it_behaves_like 'audit event contains custom message' do
-        let(:audit_events_url) { admin_audit_logs_path }
-      end
     end
   end
 

ee/spec/features/projects/feature_flags/user_creates_feature_flag_spec.rb

@@ -38,7 +38,7 @@ describe 'User creates feature flag', :js do
     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
 

ee/spec/features/projects/feature_flags/user_deletes_feature_flag_spec.rb

@@ -33,6 +33,6 @@ describe 'User deletes feature flag', :js do
   it 'records audit event' do
     visit(project_audit_events_path(project))
 
-    expect(page).to have_text("Deleted feature flag ci live trace.")
+    expect(page).to have_text("Deleted feature flag ci_live_trace.")
   end
 end

ee/spec/features/projects/feature_flags/user_updates_feature_flag_spec.rb

@@ -65,7 +65,7 @@ describe 'User updates feature flag', :js 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.")
+        have_text("Updated feature flag ci_live_trace. Updated rule review/* active state from true to false.")
       )
     end
   end
@@ -96,7 +96,7 @@ describe 'User updates feature flag', :js do
       visit(project_audit_events_path(project))
 
       expect(page).to(
-        have_text("Updated feature flag ci live trace")
+        have_text("Updated feature flag ci_live_trace")
       )
     end
   end
@@ -124,7 +124,7 @@ describe 'User updates feature flag', :js do
       visit(project_audit_events_path(project))
 
       expect(page).to(
-        have_text("Updated feature flag ci live trace")
+        have_text("Updated feature flag ci_live_trace")
       )
     end
   end

ee/spec/helpers/audit_events_helper_spec.rb

@@ -2,11 +2,12 @@ require 'spec_helper'
 
 describe AuditEventsHelper do
   describe '#human_text' do
+    let(:target_type) { 'User' }
     let(:details) do
       {
         author_name: 'John Doe',
         target_id: 1,
-        target_type: 'User',
+        target_type: target_type,
         target_details: 'Michael'
       }
     end
@@ -29,14 +30,6 @@ describe AuditEventsHelper do
       it 'returns custom message' do
         expect(subject).to eq(custom_message)
       end
-
-      context 'when custom message contains "_"' do
-        let(:custom_message) { "message_with_spaces" }
-
-        it 'replace them with spaces' do
-          expect(subject).to eq("message with spaces")
-        end
-      end
     end
   end
 

ee/spec/lib/audit/details_spec.rb

@@ -47,6 +47,7 @@ describe Audit::Details do
       let(:user_member) { create(:user) }
       let(:group) { create(:group) }
       let(:member) { create(:group_member, group: group, user: user_member) }
+      let(:target_type) { 'User' }
       let(:member_access_action) do
         {
           change: 'access_level',
@@ -54,17 +55,37 @@ describe Audit::Details do
           to: member.human_access,
           author_name: user.name,
           target_id: member.id,
-          target_type: 'User',
+          target_type: target_type,
           target_details: member.user.name
         }
       end
 
+      context 'when the target_type is not Operations::FeatureFlag' do
         it 'humanizes add group member access action' do
           string = described_class.humanize(member_access_action)
 
           expect(string).to eq('Changed access level from Guest to Owner')
         end
       end
+    end
+
+    context 'failed_login' do
+      let(:feature_flag) do
+        {
+          failed_login: 'google_oauth2',
+          author_name: 'username',
+          target_details: 'testuser',
+          ip_address: '127.0.0.1'
+        }
+      end
+      let(:message) { 'Failed to login with GOOGLE authentication' }
+
+      it 'shows the correct failed login meessage' do
+        string = described_class.humanize(feature_flag)
+
+        expect(string).to eq message
+      end
+    end
 
     context 'deploy key' do
       let(:removal_action) do

ee/spec/services/feature_flags/create_service_spec.rb

@@ -47,7 +47,7 @@ describe FeatureFlags::CreateService do
       end
 
       it 'creates audit event' do
-        expected_message = "Created feature flag <strong>feature flag</strong> "\
+        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>."

ee/spec/services/feature_flags/destroy_service_spec.rb

@@ -21,7 +21,7 @@ describe FeatureFlags::DestroyService do
 
     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>.")
+      expect(audit_event_message).to eq("Deleted feature flag <strong>#{feature_flag.name}</strong>.")
     end
 
     context 'when feature flag can not be destroyed' do

ee/spec/services/feature_flags/update_service_spec.rb

@@ -23,9 +23,9 @@ describe FeatureFlags::UpdateService do
 
       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> "\
-           "to <strong>\"new name\"</strong>.")
+        eq("Updated feature flag <strong>new_name</strong>. "\
+           "Updated name from <strong>\"#{name_was}\"</strong> "\
+           "to <strong>\"new_name\"</strong>.")
       )
     end
 

ee/spec/support/shared_examples/features/audit_event_contains_custom_message.rb

@@ -20,8 +20,8 @@ shared_examples 'audit event contains custom message' do
     visit audit_events_url
   end
 
-  it 'user sess this message' do
-    expect(page).to have_content('Message with spaces')
+  it 'user sees this message' do
+    expect(page).to have_content('Message_with_spaces')
   end
 
   context 'when it contains tags' do