Commit 2b52036b authored by Bob Van Landuyt's avatar Bob Van Landuyt

Merge branch 'convert-recaptcha-experiment-to-feature' into 'master'

Remove ReCAPTCHA experiment code

See merge request gitlab-org/gitlab!47414
parents 3ee16954 6444383c
...@@ -3,7 +3,7 @@ ...@@ -3,7 +3,7 @@
class RegistrationsController < Devise::RegistrationsController class RegistrationsController < Devise::RegistrationsController
include Recaptcha::Verify include Recaptcha::Verify
include AcceptsPendingInvitations include AcceptsPendingInvitations
include RecaptchaExperimentHelper include RecaptchaHelper
include InvisibleCaptchaOnSignup include InvisibleCaptchaOnSignup
BLOCKED_PENDING_APPROVAL_STATE = 'blocked_pending_approval'.freeze BLOCKED_PENDING_APPROVAL_STATE = 'blocked_pending_approval'.freeze
...@@ -176,5 +176,3 @@ class RegistrationsController < Devise::RegistrationsController ...@@ -176,5 +176,3 @@ class RegistrationsController < Devise::RegistrationsController
@invite_email = ActionController::Base.helpers.sanitize(params[:invite_email]) @invite_email = ActionController::Base.helpers.sanitize(params[:invite_email])
end end
end end
RegistrationsController.prepend_if_ee('EE::RegistrationsController')
# frozen_string_literal: true # frozen_string_literal: true
module RecaptchaExperimentHelper module RecaptchaHelper
def show_recaptcha_sign_up? def show_recaptcha_sign_up?
!!Gitlab::Recaptcha.enabled? !!Gitlab::Recaptcha.enabled?
end end
end end
RecaptchaExperimentHelper.prepend_if_ee('EE::RecaptchaExperimentHelper')
# frozen_string_literal: true
module EE
module RegistrationsController
extend ActiveSupport::Concern
extend ::Gitlab::Utils::Override
private
override :user_created_message
def user_created_message(confirmed: false)
experiments = "experiment_growth_recaptcha?#{show_recaptcha_sign_up?}"
super(confirmed: confirmed) + ", experiments:#{experiments}"
end
end
end
# frozen_string_literal: true
module EE
module FlipperSessionHelper
def flipper_session
@flipper_session ||= flipper_session_set? ? get_flipper_session : new_flipper_session
end
private
def flipper_session_set?
session.has_key?(FlipperSession::SESSION_KEY)
end
def get_flipper_session
FlipperSession.new(session[FlipperSession::SESSION_KEY])
end
def new_flipper_session
FlipperSession.new.tap do |flipper_session|
session[FlipperSession::SESSION_KEY] = flipper_session.id
end
end
end
end
# frozen_string_literal: true
module EE
module RecaptchaExperimentHelper
include FlipperSessionHelper
extend ::Gitlab::Utils::Override
EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME = :experiment_growth_recaptcha
override :show_recaptcha_sign_up?
def show_recaptcha_sign_up?
super && experiment_enabled_for_session?
end
private
def experiment_enabled_for_session?
::Feature.enabled?(EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME, flipper_session,
default_enabled: true)
end
end
end
---
name: experiment_growth_recaptcha
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/14019
rollout_issue_url:
milestone: '12.1'
type: development
group: group::acquisition
default_enabled: true
# frozen_string_literal: true
require 'securerandom'
class FlipperSession
SESSION_KEY = '_flipper_id'.freeze
attr_reader :id
def initialize(id = generate_id)
@id = id
end
# This method is required by Flipper
# https://github.com/jnunemaker/flipper/blob/master/docs/Gates.md#2-individual-actor
def flipper_id
"flipper_session:#{id}"
end
private
def generate_id
SecureRandom.uuid
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe RegistrationsController do
let_it_be(:user) { create(:user) }
describe '#create' do
let(:base_user_params) { build_stubbed(:user).slice(:first_name, :last_name, :username, :email, :password) }
let(:user_params) { { user: base_user_params } }
context 'when reCAPTCHA experiment enabled' do
it "logs a 'User Created' message including the experiment state" do
allow_any_instance_of(EE::RecaptchaExperimentHelper).to receive(:show_recaptcha_sign_up?).and_return(true)
expect(Gitlab::AppLogger).to receive(:info).with(/\AUser Created: .+experiment_growth_recaptcha\?true\z/).and_call_original
post :create, params: user_params
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe EE::FlipperSessionHelper do
describe '.flipper_session' do
let(:session) { {} }
before do
allow(helper).to receive(:session).and_return(session)
end
subject { helper.flipper_session }
context 'when a FlipperSession has not be previously set' do
let(:predictable_id) { 'abc123' }
before do
allow_any_instance_of(FlipperSession).to receive(:generate_id).and_return(predictable_id)
end
it 'returns an instance of FlipperSession' do
expect(subject).to be_instance_of(FlipperSession)
end
it 'sets a predictable FlipperSession id to session' do
subject
expect(session).to include(FlipperSession::SESSION_KEY => predictable_id)
end
end
context 'when a FlipperSession has been previously set' do
let(:predictable_id) { 'def456' }
before do
allow_any_instance_of(FlipperSession).to receive(:generate_id).and_return(predictable_id)
end
it 'returns a FlipperSession with the same ID' do
# Cannot call subject as it will be cached and not trigger the logic a second time
existing_flipper_session = helper.flipper_session
expect(subject.id).to eq(existing_flipper_session.id)
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe RecaptchaExperimentHelper, type: :helper do
using RSpec::Parameterized::TableSyntax
let(:session) { {} }
before do
allow(helper).to receive(:session) { session }
end
describe '.show_recaptcha_sign_up?' do
context 'when reCAPTCHA is disabled' do
it 'returns false' do
stub_application_setting(recaptcha_enabled: false)
expect(helper.show_recaptcha_sign_up?).to be(false)
end
end
context 'when reCAPTCHA is enabled' do
before do
stub_application_setting(recaptcha_enabled: true)
end
context 'and experiment_growth_recaptcha has not been set' do
it 'returns true' do
expect(helper.show_recaptcha_sign_up?).to be(true)
end
end
context 'and experiment_growth_recaptcha has been set' do
let(:feature_name) { described_class::EXPERIMENT_GROWTH_RECAPTCHA_FEATURE_NAME }
where(:flipper_session_id, :expected_result) do
'00687625-667c-480c-ae2a-9bf861ddd909' | true
'b8b78156-f7b8-4bf4-b906-06a899b84ea3' | false
'e622a262-6e48-41ba-b19f-2d91c24f17a3' | true
'd2c0aae1-bc08-4000-bbb2-0dd2802f67e2' | false
end
with_them do
before do
# Enable feature to 50% of actors
Feature.enable_percentage_of_actors(feature_name, 50)
end
it "returns expected_result" do
allow(helper).to receive(:flipper_session).and_return(FlipperSession.new(flipper_session_id))
expect(helper.show_recaptcha_sign_up?).to eq(expected_result)
end
end
end
end
end
end
# frozen_string_literal: true
require 'fast_spec_helper'
RSpec.describe FlipperSession do
describe '#flipper_id' do
context 'without passing in an ID' do
it 'returns a flipper_session:UUID' do
expect(described_class.new.flipper_id).to match(/\Aflipper_session:\w{8}-\w{4}-\w{4}-\w{4}-\w{12}\z/)
end
end
context 'passing in an ID' do
it 'returns a flipper_session:def456' do
id = 'abc123'
expect(described_class.new(id).flipper_id).to eq("flipper_session:#{id}")
end
end
end
end
...@@ -39,12 +39,6 @@ tests = [ ...@@ -39,12 +39,6 @@ tests = [
expected: ['spec/lib/gitaly/server_spec.rb'] expected: ['spec/lib/gitaly/server_spec.rb']
}, },
{
explanation: 'EE lib should map to respective spec',
source: 'ee/lib/flipper_session.rb',
expected: ['ee/spec/lib/flipper_session_spec.rb']
},
{ {
explanation: 'Tooling should map to respective spec', explanation: 'Tooling should map to respective spec',
source: 'tooling/lib/tooling/test_file_finder.rb', source: 'tooling/lib/tooling/test_file_finder.rb',
......
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe RecaptchaExperimentHelper, type: :helper do RSpec.describe RecaptchaHelper, type: :helper do
let(:session) { {} } let(:session) { {} }
before do before do
......
...@@ -63,14 +63,6 @@ RSpec.describe Tooling::TestFileFinder do ...@@ -63,14 +63,6 @@ RSpec.describe Tooling::TestFileFinder do
end end
end end
context 'when given a lib file in ee/' do
let(:file) { 'ee/lib/flipper_session.rb' }
it 'returns the matching ee/ lib test file' do
expect(subject.test_files).to contain_exactly('ee/spec/lib/flipper_session_spec.rb')
end
end
context 'when given a test file in ee/' do context 'when given a test file in ee/' do
let(:file) { 'ee/spec/models/container_registry/event_spec.rb' } let(:file) { 'ee/spec/models/container_registry/event_spec.rb' }
......
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