Commit ada28f94 authored by Jan Provaznik's avatar Jan Provaznik

Merge branch '270409-experiment-cleanup-discover-security' into 'master'

Cleanup "discover security" experiment

See merge request gitlab-org/gitlab!45636
parents 4d09efc0 c7cde391
# frozen_string_literal: true # frozen_string_literal: true
class UserPreference < ApplicationRecord class UserPreference < ApplicationRecord
include IgnorableColumns
# We could use enums, but Rails 4 doesn't support multiple # We could use enums, but Rails 4 doesn't support multiple
# enum options with same name for multiple fields, also it creates # enum options with same name for multiple fields, also it creates
# extra methods that aren't really needed here. # extra methods that aren't really needed here.
NOTES_FILTERS = { all_notes: 0, only_comments: 1, only_activity: 2 }.freeze NOTES_FILTERS = { all_notes: 0, only_comments: 1, only_activity: 2 }.freeze
ignore_column :feature_filter_type, remove_with: '13.8', remove_after: '2021-01-22'
belongs_to :user belongs_to :user
scope :with_user, -> { joins(:user) } scope :with_user, -> { joins(:user) }
......
---
title: Enable dashboard security discover button and ignore feature_filter_type column
cleanup
merge_request: 45636
author:
type: added
...@@ -77,8 +77,7 @@ module EE ...@@ -77,8 +77,7 @@ module EE
!!current_user && !!current_user &&
::Gitlab.com? && ::Gitlab.com? &&
!@group.feature_available?(:security_dashboard) && !@group.feature_available?(:security_dashboard) &&
can?(current_user, :admin_group, @group) && can?(current_user, :admin_group, @group)
current_user.ab_feature_enabled?(:discover_security)
end end
def show_group_activity_analytics? def show_group_activity_analytics?
......
...@@ -244,8 +244,7 @@ module EE ...@@ -244,8 +244,7 @@ module EE
!!current_user && !!current_user &&
::Gitlab.com? && ::Gitlab.com? &&
!project.feature_available?(:security_dashboard) && !project.feature_available?(:security_dashboard) &&
can?(current_user, :admin_namespace, project.root_ancestor) && can?(current_user, :admin_namespace, project.root_ancestor)
current_user.ab_feature_enabled?(:discover_security)
end end
override :can_import_members? override :can_import_members?
......
...@@ -308,29 +308,6 @@ module EE ...@@ -308,29 +308,6 @@ module EE
super super
end end
def ab_feature_enabled?(feature, percentage: nil)
return false unless ::Gitlab.com?
return false if ::Gitlab::Geo.secondary?
raise "Currently only discover_security feature is supported" unless feature == :discover_security
return false unless ::Feature.enabled?(feature)
filter = user_preference.feature_filter_type.presence || 0
# We use a 2nd feature flag for control as enabled and percentage_of_time for chatops
flipper_feature = ::Feature.get((feature.to_s + '_control').to_sym) # rubocop:disable Gitlab/AvoidFeatureGet
percentage ||= flipper_feature&.percentage_of_time_value || 0
return false if percentage <= 0
if filter == UserPreference::FEATURE_FILTER_UNKNOWN
filter = SecureRandom.rand * 100 <= percentage ? UserPreference::FEATURE_FILTER_EXPERIMENT : UserPreference::FEATURE_FILTER_CONTROL
user_preference.update_column :feature_filter_type, filter
end
filter == UserPreference::FEATURE_FILTER_EXPERIMENT
end
def gitlab_employee? def gitlab_employee?
strong_memoize(:gitlab_employee) do strong_memoize(:gitlab_employee) do
::Gitlab.com? && ::Feature.enabled?(:gitlab_employee_badge) && gitlab_team_member? ::Gitlab.com? && ::Feature.enabled?(:gitlab_employee_badge) && gitlab_team_member?
......
...@@ -11,9 +11,5 @@ module EE ...@@ -11,9 +11,5 @@ module EE
validates :epic_notes_filter, inclusion: { in: ::UserPreference::NOTES_FILTERS.values }, presence: true validates :epic_notes_filter, inclusion: { in: ::UserPreference::NOTES_FILTERS.values }, presence: true
end end
FEATURE_FILTER_UNKNOWN = 0
FEATURE_FILTER_CONTROL = 1
FEATURE_FILTER_EXPERIMENT = 2
end end
end end
---
name: discover_security
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/21658
rollout_issue_url: https://gitlab.com/gitlab-org/growth/team-tasks/-/issues/54
group: group::conversion
type: development
default_enabled: false
...@@ -130,10 +130,8 @@ RSpec.describe GroupsHelper do ...@@ -130,10 +130,8 @@ RSpec.describe GroupsHelper do
using RSpec::Parameterized::TableSyntax using RSpec::Parameterized::TableSyntax
where( where(
ab_feature_enabled?: [true, false],
gitlab_com?: [true, false], gitlab_com?: [true, false],
user?: [true, false], user?: [true, false],
discover_security_feature_enabled?: [true, false],
security_dashboard_feature_available?: [true, false], security_dashboard_feature_available?: [true, false],
can_admin_group?: [true, false] can_admin_group?: [true, false]
) )
...@@ -142,13 +140,10 @@ RSpec.describe GroupsHelper do ...@@ -142,13 +140,10 @@ RSpec.describe GroupsHelper do
it 'returns the expected value' do it 'returns the expected value' do
allow(helper).to receive(:current_user) { user? ? owner : nil } allow(helper).to receive(:current_user) { user? ? owner : nil }
allow(::Gitlab).to receive(:com?) { gitlab_com? } allow(::Gitlab).to receive(:com?) { gitlab_com? }
allow(owner).to receive(:ab_feature_enabled?) { ab_feature_enabled? }
allow(::Feature).to receive(:enabled?).with(:discover_security) { discover_security_feature_enabled? }
allow(group).to receive(:feature_available?) { security_dashboard_feature_available? } allow(group).to receive(:feature_available?) { security_dashboard_feature_available? }
allow(helper).to receive(:can?) { can_admin_group? } allow(helper).to receive(:can?) { can_admin_group? }
expected_value = user? && gitlab_com? && expected_value = user? && gitlab_com? && !security_dashboard_feature_available? && can_admin_group?
ab_feature_enabled? && !security_dashboard_feature_available? && can_admin_group?
expect(helper.show_discover_group_security?(group)).to eq(expected_value) expect(helper.show_discover_group_security?(group)).to eq(expected_value)
end end
......
...@@ -292,7 +292,6 @@ RSpec.describe ProjectsHelper do ...@@ -292,7 +292,6 @@ RSpec.describe ProjectsHelper do
let(:user) { create(:user) } let(:user) { create(:user) }
where( where(
ab_feature_enabled?: [true, false],
gitlab_com?: [true, false], gitlab_com?: [true, false],
user?: [true, false], user?: [true, false],
security_dashboard_feature_available?: [true, false], security_dashboard_feature_available?: [true, false],
...@@ -302,13 +301,11 @@ RSpec.describe ProjectsHelper do ...@@ -302,13 +301,11 @@ RSpec.describe ProjectsHelper do
with_them do with_them do
it 'returns the expected value' do it 'returns the expected value' do
allow(::Gitlab).to receive(:com?) { gitlab_com? } allow(::Gitlab).to receive(:com?) { gitlab_com? }
allow(user).to receive(:ab_feature_enabled?) { ab_feature_enabled? }
allow(helper).to receive(:current_user) { user? ? user : nil } allow(helper).to receive(:current_user) { user? ? user : nil }
allow(project).to receive(:feature_available?) { security_dashboard_feature_available? } allow(project).to receive(:feature_available?) { security_dashboard_feature_available? }
allow(helper).to receive(:can?) { can_admin_namespace? } allow(helper).to receive(:can?) { can_admin_namespace? }
expected_value = user? && gitlab_com? && expected_value = user? && gitlab_com? && !security_dashboard_feature_available? && can_admin_namespace?
ab_feature_enabled? && !security_dashboard_feature_available? && can_admin_namespace?
expect(helper.show_discover_project_security?(project)).to eq(expected_value) expect(helper.show_discover_project_security?(project)).to eq(expected_value)
end end
......
...@@ -907,126 +907,6 @@ RSpec.describe User do ...@@ -907,126 +907,6 @@ RSpec.describe User do
end end
end end
describe '#ab_feature_enabled?' do
let(:experiment_user) { create(:user) }
let(:new_user) { create(:user) }
let(:new_fresh_user) { create(:user) }
let(:control_user) { create(:user) }
let(:users_of_different_groups) { [experiment_user, new_user, new_fresh_user, control_user] }
before do
create(:user_preference, user: experiment_user, feature_filter_type: UserPreference::FEATURE_FILTER_EXPERIMENT)
create(:user_preference, user: new_user, feature_filter_type: UserPreference::FEATURE_FILTER_UNKNOWN)
create(:user_preference, user: new_fresh_user, feature_filter_type: nil)
create(:user_preference, user: control_user, feature_filter_type: UserPreference::FEATURE_FILTER_CONTROL)
end
context 'when not on Gitlab.com' do
before do
allow(Gitlab).to receive(:com?).and_return(false)
end
it 'returns false' do
users_of_different_groups.each do |user|
expect(user.ab_feature_enabled?(:discover_security, percentage: 100)).to eq(false)
end
end
end
context 'when on Gitlab.com' do
before do
allow(Gitlab).to receive(:com?).and_return(true)
end
context 'when on a secondary Geo' do
before do
allow(Gitlab::Geo).to receive(:secondary?).and_return(true)
end
it 'returns false' do
users_of_different_groups.each do |user|
expect(user.ab_feature_enabled?(:discover_security, percentage: 100)).to eq(false)
end
end
end
context 'when not on a secondary Geo' do
before do
allow(Gitlab::Geo).to receive(:secondary?).and_return(false)
end
context 'for any feature except discover_security' do
it 'raises runtime error' do
users_of_different_groups.each do |user|
expect do
user.ab_feature_enabled?(:any_other_feature, percentage: 100)
end.to raise_error(RuntimeError, 'Currently only discover_security feature is supported')
end
end
end
context 'when discover_security feature flag is disabled' do
before do
stub_feature_flags(discover_security: false)
end
it 'returns false' do
users_of_different_groups.each do |user|
expect(user.ab_feature_enabled?(:discover_security, percentage: 100)).to eq(false)
end
end
end
context 'when discover_security feature flag is enabled' do
it 'returns false when in control group' do
expect(control_user.ab_feature_enabled?(:discover_security, percentage: 100)).to eq(false)
end
it 'returns true for experiment group' do
expect(experiment_user.ab_feature_enabled?(:discover_security, percentage: 100)).to eq(true)
end
it 'assigns to control or experiment group when feature_filter_type is nil' do
new_fresh_user.ab_feature_enabled?(:discover_security, percentage: 100)
expect(new_fresh_user.user_preference.feature_filter_type).not_to eq(UserPreference::FEATURE_FILTER_UNKNOWN)
end
it 'assigns to control or experiment group when feature_filter_type is zero' do
new_user.ab_feature_enabled?(:discover_security, percentage: 100)
expect(new_user.user_preference.feature_filter_type).not_to eq(UserPreference::FEATURE_FILTER_UNKNOWN)
end
it 'returns false for zero percentage' do
expect(experiment_user.ab_feature_enabled?(:discover_security, percentage: 0)).to eq(false)
end
it 'returns false when no percentage is provided' do
expect(experiment_user.ab_feature_enabled?(:discover_security)).to eq(false)
end
it 'returns true when 100% control percentage is provided' do
Feature.enable_percentage_of_time(:discover_security_control, 100)
expect(experiment_user.ab_feature_enabled?(:discover_security)).to eq(true)
expect(experiment_user.user_preference.feature_filter_type).to eq(UserPreference::FEATURE_FILTER_EXPERIMENT)
end
it 'returns false if flipper returns nil for non-existing feature' do
# The following setup ensures that if the Feature interface changes
# it does not break any user-facing screens
allow(Feature).to receive(:get).with(:discover_security).and_return(nil)
allow(Feature).to receive(:enabled?).and_return(true)
allow(Feature).to receive(:get).with(:discover_security_control).and_return(nil)
expect(experiment_user.ab_feature_enabled?(:discover_security)).to eq(false)
end
end
end
end
end
describe '#manageable_groups_eligible_for_subscription' do describe '#manageable_groups_eligible_for_subscription' do
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
let_it_be(:licensed_group) { create(:group, gitlab_subscription: create(:gitlab_subscription, :bronze)) } let_it_be(:licensed_group) { create(:group, gitlab_subscription: create(:gitlab_subscription, :bronze)) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment