Commit 63d389e9 authored by Alex Buijs's avatar Alex Buijs

Implement feedback fixes BE

- redirect users to almost there page after signing up when
soft_email_confirmation is not enabled
- remove enabled_since code for feature flags, since it is unreliable
- add User#role_required? and User#set_role_required! methods to
‘abuse’ the role column in order to determine whether the role is
required
- store the requested url and redirect there after setting the role
- add flash notice after successful signup
- enable invisible captcha when signup_flow experiment is enabled
- change `Welcome to GitLab.com` text to `Welcome to GitLab` in
order to apply to Self hosted instances as well
- cancel the signup_notice flash in the create action, where it is set
parent ed071fe0
......@@ -553,11 +553,9 @@ class ApplicationController < ActionController::Base
# A user is redirected to the welcome page when his role is still blank, his name is equal to his username,
# the experiment is enabled for the current user and the user was created after the experiment was initiated.
def require_role
return unless current_user &&
current_user.role.blank? &&
current_user.name == current_user.username &&
experiment_enabled?(:signup_flow) &&
experiment_enabled_since(:signup_flow) < current_user.created_at
return unless current_user && current_user.role_required? && experiment_enabled?(:signup_flow)
store_location_for :user, request.fullpath
redirect_to users_sign_up_welcome_path
end
......
......@@ -8,7 +8,7 @@ module InvisibleCaptcha
end
def on_honeypot_spam_callback
return unless Feature.enabled?(:invisible_captcha)
return unless Feature.enabled?(:invisible_captcha) || experiment_enabled?(:signup_flow)
invisible_captcha_honeypot_counter.increment
log_request('Invisible_Captcha_Honeypot_Request')
......@@ -17,7 +17,7 @@ module InvisibleCaptcha
end
def on_timestamp_spam_callback
return unless Feature.enabled?(:invisible_captcha)
return unless Feature.enabled?(:invisible_captcha) || experiment_enabled?(:signup_flow)
invisible_captcha_timestamp_counter.increment
log_request('Invisible_Captcha_Timestamp_Request')
......
......@@ -27,8 +27,13 @@ class RegistrationsController < Devise::RegistrationsController
super do |new_user|
persist_accepted_terms_if_required(new_user)
set_role_required(new_user)
yield new_user if block_given?
end
# Do not show the signed_up notice message when the signup_flow experiment is enabled.
# Instead, show it after succesfully updating the role.
flash[:notice] = nil if experiment_enabled?(:signup_flow)
rescue Gitlab::Access::AccessDeniedError
redirect_to(new_user_session_path)
end
......@@ -45,9 +50,8 @@ class RegistrationsController < Devise::RegistrationsController
def welcome
return redirect_to new_user_registration_path unless current_user
return redirect_to stored_location_or_dashboard(current_user) if current_user.role.present?
return redirect_to stored_location_or_dashboard_or_almost_there_path(current_user) if current_user.role.present?
flash[:notice] = nil
current_user.name = nil
render layout: 'devise_experimental_separate_sign_up_flow'
end
......@@ -57,7 +61,8 @@ class RegistrationsController < Devise::RegistrationsController
result = ::Users::UpdateService.new(current_user, user_params.merge(user: current_user)).execute
if result[:status] == :success
redirect_to stored_location_or_dashboard(current_user)
set_flash_message! :notice, :signed_up
redirect_to stored_location_or_dashboard_or_almost_there_path(current_user)
else
redirect_to users_sign_up_welcome_path, alert: result[:message]
end
......@@ -75,6 +80,10 @@ class RegistrationsController < Devise::RegistrationsController
end
end
def set_role_required(new_user)
new_user.set_role_required! if new_user.persisted? && experiment_enabled?(:signup_flow)
end
def destroy_confirmation_valid?
if current_user.confirm_deletion_with_password?
current_user.valid_password?(params[:password])
......@@ -100,7 +109,7 @@ class RegistrationsController < Devise::RegistrationsController
return users_sign_up_welcome_path if experiment_enabled?(:signup_flow)
confirmed_or_unconfirmed_access_allowed(user) ? stored_location_or_dashboard(user) : users_almost_there_path
stored_location_or_dashboard_or_almost_there_path(user)
end
def after_inactive_sign_up_path_for(resource)
......@@ -174,13 +183,17 @@ class RegistrationsController < Devise::RegistrationsController
end
def confirmed_or_unconfirmed_access_allowed(user)
user.confirmed? || Feature.enabled?(:soft_email_confirmation)
user.confirmed? || Feature.enabled?(:soft_email_confirmation) || experiment_enabled?(:signup_flow)
end
def stored_location_or_dashboard(user)
stored_location_for(user) || dashboard_projects_path
end
def stored_location_or_dashboard_or_almost_there_path(user)
confirmed_or_unconfirmed_access_allowed(user) ? stored_location_or_dashboard(user) : users_almost_there_path
end
# Part of an experiment to build a new sign up flow. Will be resolved
# with https://gitlab.com/gitlab-org/growth/engineering/issues/64
def choose_layout
......
......@@ -233,7 +233,7 @@ class User < ApplicationRecord
# User's role
# Note: When adding an option, it MUST go on the end of the array.
enum role: [:software_developer, :development_team_lead, :devops_engineer, :systems_administrator, :security_analyst, :data_analyst, :product_manager, :product_designer, :other]
enum role: [:software_developer, :development_team_lead, :devops_engineer, :systems_administrator, :security_analyst, :data_analyst, :product_manager, :product_designer, :other], _suffix: true
delegate :path, to: :namespace, allow_nil: true, prefix: true
delegate :notes_filter_for, to: :user_preference
......@@ -1562,6 +1562,19 @@ class User < ApplicationRecord
[last_activity, last_sign_in].compact.max
end
# Below is used for the signup_flow experiment. Should be removed
# when experiment finishes. See gitlab-org/growth/engineering#64
REQUIRES_ROLE_VALUE = 99
def role_required?
role_before_type_cast == REQUIRES_ROLE_VALUE
end
def set_role_required!
update_column(:role, REQUIRES_ROLE_VALUE)
end
# End of signup_flow experiment methods
# @deprecated
alias_method :owned_or_masters_groups, :owned_or_maintainers_groups
......
- content_for(:page_title, _('Register for GitLab.com'))
- content_for(:page_title, _('Register for GitLab'))
- max_username_length = 255
.signup-box.p-3.mb-2
.signup-body
......
- content_for(:page_title, _('Welcome to GitLab.com<br>%{username}!' % { username: current_user.username }).html_safe)
- content_for(:page_title, _('Welcome to GitLab<br>%{username}!' % { username: current_user.username }).html_safe)
- max_name_length = 128
.text-center.mb-2
= _('In order to tailor your experience with GitLab<br>we would like to know a bit more about you.').html_safe
......
......@@ -15,7 +15,7 @@ module EE
end
def sign_up_params
clean_params = params.require(:user).permit(:username, :email, :email_confirmation, :name, :password, :email_opted_in)
clean_params = super.merge(params.require(:user).permit(:email_opted_in))
if clean_params[:email_opted_in] == '1'
clean_params[:email_opted_in_ip] = request.remote_ip
......@@ -23,10 +23,6 @@ module EE
clean_params[:email_opted_in_at] = Time.zone.now
end
if experiment_enabled?(:signup_flow)
clean_params[:name] = clean_params[:username]
end
clean_params
end
end
......
......@@ -18,10 +18,6 @@ class Feature
class FlipperGate < Flipper::Adapters::ActiveRecord::Gate
superclass.table_name = 'feature_gates'
def self.created_date(feature_name)
find_by(feature_key: feature_name).created_at
end
end
class << self
......@@ -93,10 +89,6 @@ class Feature
feature.remove
end
def enabled_since(key)
FlipperGate.created_date(key) if enabled?(key)
end
def flipper
if Gitlab::SafeRequestStore.active?
Gitlab::SafeRequestStore[:flipper] ||= build_flipper_instance
......
......@@ -44,10 +44,6 @@ module Gitlab
Experimentation.enabled?(experiment_key, experimentation_subject_index)
end
def experiment_enabled_since(experiment_key)
Experimentation.enabled_since(experiment_key)
end
private
def experimentation_subject_index
......@@ -72,10 +68,6 @@ module Gitlab
experiment.enabled_for_environment? &&
experiment.enabled_for_experimentation_subject?(experimentation_subject_index)
end
def enabled_since(experiment_key)
experiment(experiment_key).feature_toggle_enabled_since
end
end
Experiment = Struct.new(:key, :feature_toggle, :environment, :enabled_ratio, keyword_init: true) do
......@@ -85,10 +77,6 @@ module Gitlab
Feature.enabled?(feature_toggle)
end
def feature_toggle_enabled_since
Feature.enabled_since(feature_toggle)
end
def enabled_for_environment?
return true if environment.nil?
......
......@@ -13453,7 +13453,7 @@ msgstr ""
msgid "Register and see your runners for this project."
msgstr ""
msgid "Register for GitLab.com"
msgid "Register for GitLab"
msgstr ""
msgid "Register now"
......
......@@ -848,42 +848,42 @@ describe ApplicationController do
def index; end
end
let(:user) { create(:user) }
let(:experiment_enabled) { true }
before do
stub_experiment(signup_flow: experiment_enabled)
allow(Gitlab::Experimentation).to receive(:enabled_since).with(:signup_flow).and_return(1.day.ago)
sign_in(user)
get :index
end
context 'all the right conditions' do
let(:user) { create(:user, role: nil, name: 'some_name', username: 'some_name') }
let(:experiment_enabled) { true }
context 'experiment enabled and user with required role' do
before do
user.set_role_required!
sign_in(user)
get :index
end
it { is_expected.to redirect_to users_sign_up_welcome_path }
end
context 'a user with a role' do
let(:user) { create(:user, name: 'some_name', username: 'some_name') }
it { is_expected.not_to redirect_to users_sign_up_welcome_path }
context 'experiment enabled and user without a role' do
before do
sign_in(user)
get :index
end
context 'a user with a name other than the username' do
let(:user) { create(:user, role: nil) }
it { is_expected.not_to redirect_to users_sign_up_welcome_path }
end
it { is_expected.not_to redirect_to users_sign_up_welcome_path }
end
context 'user created before the experiment started' do
let(:user) { create(:user, role: nil, name: 'some_name', username: 'some_name', created_at: 2.days.ago) }
context 'experiment disabled and user with required role' do
let(:experiment_enabled) { false }
it { is_expected.not_to redirect_to users_sign_up_welcome_path }
before do
user.set_role_required!
sign_in(user)
get :index
end
context 'experiment disabled' do
let(:experiment_enabled) { false }
it { is_expected.not_to redirect_to users_sign_up_welcome_path }
end
it { is_expected.not_to redirect_to users_sign_up_welcome_path }
end
end
end
......@@ -391,31 +391,33 @@ end
describe 'With experimental flow' do
before do
stub_experiment(signup_flow: true)
allow(Gitlab::Experimentation).to receive(:enabled_since).with(:signup_flow).and_return(1.day.ago)
end
it_behaves_like 'Signup'
describe 'user without role' do
let(:user) { create(:user, role: nil, name: 'some_name', username: 'some_name') }
describe 'when role is required' do
let(:user) { create(:user) }
before do
user.set_role_required!
user.reload
sign_in(user)
visit root_path
visit new_project_path
end
it 'is redirected to step 2 of the signup process' do
expect(page).to have_text("Welcome to GitLab.com#{user.username}!")
end
it 'updates the user and sets the name and role' do
it 'updates the user, sets the name and role and redirects to the requested url' do
fill_in 'user_name', with: 'New name'
select 'Software Developer', from: 'user_role'
click_button 'Get started!'
user.reload
expect(user.name).to eq 'New name'
expect(user.role).to eq 'software_developer'
expect(user.software_developer_role?).to be_truthy
expect(page).to have_current_path(new_project_path)
end
end
end
......@@ -254,28 +254,6 @@ describe Feature do
end
end
describe '.enabled_since' do
it 'returns nil when the feature is not enabled' do
described_class.disable(:disabled_feature_flag)
expect(described_class.enabled_since(:disabled_feature_flag)).to be_nil
end
it 'returns nil when the feature does not exist' do
expect(described_class.enabled_since(:non_existent_feature_flag)).to be_nil
end
it 'returns the created_at date when the feature is enabled' do
created_time = 1.day.ago
travel_to(created_time) do
described_class.enable(:enabled_feature_flag)
end
expect(described_class.enabled_since(:enabled_feature_flag)).to be_within(1.second).of(created_time)
end
end
describe '.remove' do
context 'for a non-persisted feature' do
it 'returns nil' do
......
......@@ -54,16 +54,6 @@ describe Gitlab::Experimentation::ControllerConcern, type: :controller do
end
end
end
describe '#experiment_enabled_since' do
it 'returns the enabled_since time of the experiment' do
enabled_time = 1.day.ago
allow(Gitlab::Experimentation).to receive(:enabled_since).with(:test_experiment).and_return(enabled_time)
expect(controller.experiment_enabled_since(:test_experiment)).to eq(enabled_time)
end
end
end
describe Gitlab::Experimentation do
......@@ -163,14 +153,4 @@ describe Gitlab::Experimentation do
end
end
end
describe '.feature_toggle_enabled_since' do
it 'returns the enabled_since time of the feature' do
enabled_since_time = 1.day.ago
allow(Feature).to receive(:enabled_since).with(:test_experiment_toggle).and_return(enabled_since_time)
expect(described_class.enabled_since(:test_experiment)).to eq(enabled_since_time)
end
end
end
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