diff options
36 files changed, 380 insertions, 107 deletions
diff --git a/app/assets/javascripts/pages/registrations/welcome/index.js b/app/assets/javascripts/pages/registrations/welcome/index.js new file mode 100644 index 00000000000..2d555fa7977 --- /dev/null +++ b/app/assets/javascripts/pages/registrations/welcome/index.js @@ -0,0 +1,7 @@ +import LengthValidator from '~/pages/sessions/new/length_validator'; +import NoEmojiValidator from '~/emoji/no_emoji_validator'; + +document.addEventListener('DOMContentLoaded', () => { + new LengthValidator(); // eslint-disable-line no-new + new NoEmojiValidator(); // eslint-disable-line no-new +}); diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index f5939b61948..1443a71f6b1 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -29,6 +29,7 @@ class ApplicationController < ActionController::Base before_action :active_user_check, unless: :devise_controller? before_action :set_usage_stats_consent_flag before_action :check_impersonation_availability + before_action :require_role around_action :set_locale around_action :set_session_storage @@ -547,6 +548,16 @@ class ApplicationController < ActionController::Base def current_user_mode @current_user_mode ||= Gitlab::Auth::CurrentUserMode.new(current_user) end + + # A user requires a role when they are part of the experimental signup flow (executed by the Growth team). Users + # are redirected to the welcome page when their role is required and the experiment is enabled for the current user. + def require_role + 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 end ApplicationController.prepend_if_ee('EE::ApplicationController') diff --git a/app/controllers/concerns/invisible_captcha.rb b/app/controllers/concerns/invisible_captcha.rb index 45c0a5c58ef..d56f1d7fa5f 100644 --- a/app/controllers/concerns/invisible_captcha.rb +++ b/app/controllers/concerns/invisible_captcha.rb @@ -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') diff --git a/app/controllers/oauth/applications_controller.rb b/app/controllers/oauth/applications_controller.rb index ab4ca56bb49..12dc2d1af1c 100644 --- a/app/controllers/oauth/applications_controller.rb +++ b/app/controllers/oauth/applications_controller.rb @@ -5,6 +5,7 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController include Gitlab::Allowable include PageLayoutHelper include OauthApplications + include Gitlab::Experimentation::ControllerConcern before_action :verify_user_oauth_applications_enabled, except: :index before_action :authenticate_user! diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb index 705389749d8..e65726dffbf 100644 --- a/app/controllers/oauth/authorizations_controller.rb +++ b/app/controllers/oauth/authorizations_controller.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class Oauth::AuthorizationsController < Doorkeeper::AuthorizationsController + include Gitlab::Experimentation::ControllerConcern layout 'profile' # Overridden from Doorkeeper::AuthorizationsController to diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb index 958a24b6c0e..2b7571e42b7 100644 --- a/app/controllers/profiles_controller.rb +++ b/app/controllers/profiles_controller.rb @@ -100,6 +100,7 @@ class ProfilesController < Profiles::ApplicationController :avatar, :bio, :email, + :role, :hide_no_password, :hide_no_ssh_key, :hide_project_limit, diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 3a2975e92d6..4a746fc915d 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -8,13 +8,14 @@ class RegistrationsController < Devise::RegistrationsController layout :choose_layout + skip_before_action :require_role, only: [:welcome, :update_role] prepend_before_action :check_captcha, only: :create before_action :whitelist_query_limiting, only: [:destroy] before_action :ensure_terms_accepted, if: -> { action_name == 'create' && Gitlab::CurrentSettings.current_application_settings.enforce_terms? } def new - if helpers.use_experimental_separate_sign_up_flow? + if experiment_enabled?(:signup_flow) @resource = build_resource else redirect_to new_user_session_path(anchor: 'register-pane') @@ -26,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 @@ -42,6 +48,26 @@ class RegistrationsController < Devise::RegistrationsController end end + def welcome + return redirect_to new_user_registration_path unless current_user + return redirect_to stored_location_or_dashboard_or_almost_there_path(current_user) if current_user.role.present? + + current_user.name = nil + render layout: 'devise_experimental_separate_sign_up_flow' + end + + def update_role + user_params = params.require(:user).permit(:name, :role) + result = ::Users::UpdateService.new(current_user, user_params.merge(user: current_user)).execute + + if result[:status] == :success + 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 + end + protected def persist_accepted_terms_if_required(new_user) @@ -54,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]) @@ -76,7 +106,10 @@ class RegistrationsController < Devise::RegistrationsController def after_sign_up_path_for(user) Gitlab::AppLogger.info(user_created_message(confirmed: user.confirmed?)) - confirmed_or_unconfirmed_access_allowed(user) ? stored_location_or_dashboard(user) : users_almost_there_path + + return users_sign_up_welcome_path if experiment_enabled?(:signup_flow) + + stored_location_or_dashboard_or_almost_there_path(user) end def after_inactive_sign_up_path_for(resource) @@ -103,6 +136,7 @@ class RegistrationsController < Devise::RegistrationsController ensure_correct_params! return unless Feature.enabled?(:registrations_recaptcha, default_enabled: true) # reCAPTCHA on the UI will still display however + return if experiment_enabled?(:signup_flow) # when the experimental signup flow is enabled for the current user, disable the reCAPTCHA check return unless show_recaptcha_sign_up? return unless Gitlab::Recaptcha.load_configurations! @@ -114,7 +148,13 @@ class RegistrationsController < Devise::RegistrationsController end def sign_up_params - params.require(:user).permit(:username, :email, :email_confirmation, :name, :password) + clean_params = params.require(:user).permit(:username, :email, :email_confirmation, :name, :password) + + if experiment_enabled?(:signup_flow) + clean_params[:name] = clean_params[:username] + end + + clean_params end def resource_name @@ -144,17 +184,21 @@ 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 - if helpers.use_experimental_separate_sign_up_flow? + if experiment_enabled?(:signup_flow) 'devise_experimental_separate_sign_up_flow' else 'devise' diff --git a/app/helpers/sessions_helper.rb b/app/helpers/sessions_helper.rb index 2a5a3b9eac6..af98a611b8b 100644 --- a/app/helpers/sessions_helper.rb +++ b/app/helpers/sessions_helper.rb @@ -4,8 +4,4 @@ module SessionsHelper def unconfirmed_email? flash[:alert] == t(:unconfirmed, scope: [:devise, :failure]) end - - def use_experimental_separate_sign_up_flow? - ::Gitlab.dev_env_or_com? && Feature.enabled?(:experimental_separate_sign_up_flow) - end end diff --git a/app/models/user.rb b/app/models/user.rb index fe5c13f1c68..321a4080484 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -231,6 +231,10 @@ class User < ApplicationRecord # Note: When adding an option, it MUST go on the end of the array. enum project_view: [:readme, :activity, :files] + # 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], _suffix: true + delegate :path, to: :namespace, allow_nil: true, prefix: true delegate :notes_filter_for, to: :user_preference delegate :set_notes_filter, to: :user_preference @@ -1571,6 +1575,20 @@ 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 https://gitlab.com/gitlab-org/growth/engineering/issues/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 diff --git a/app/views/devise/registrations/new.html.haml b/app/views/devise/registrations/new.html.haml index dfdf7429dc5..5f85235e8fa 100644 --- a/app/views/devise/registrations/new.html.haml +++ b/app/views/devise/registrations/new.html.haml @@ -1,5 +1,5 @@ - page_title "Sign up" -- if use_experimental_separate_sign_up_flow? +- if experiment_enabled?(:signup_flow) = render 'devise/shared/experimental_separate_sign_up_flow_box' - else = render 'devise/shared/signup_box' diff --git a/app/views/devise/sessions/new.html.haml b/app/views/devise/sessions/new.html.haml index 17e5da501f5..8f6c3ecbe58 100644 --- a/app/views/devise/sessions/new.html.haml +++ b/app/views/devise/sessions/new.html.haml @@ -4,7 +4,7 @@ - if form_based_providers.any? = render 'devise/shared/tabs_ldap' - else - - unless use_experimental_separate_sign_up_flow? + - unless experiment_enabled?(:signup_flow) = render 'devise/shared/tabs_normal' .tab-content - if password_authentication_enabled_for_web? || ldap_enabled? || crowd_enabled? diff --git a/app/views/devise/shared/_experimental_separate_sign_up_flow_box.html.haml b/app/views/devise/shared/_experimental_separate_sign_up_flow_box.html.haml index f92c29da5e6..5d163d03c73 100644 --- a/app/views/devise/shared/_experimental_separate_sign_up_flow_box.html.haml +++ b/app/views/devise/shared/_experimental_separate_sign_up_flow_box.html.haml @@ -1,4 +1,4 @@ -- max_name_length = 128 +- content_for(:page_title, _('Register for GitLab')) - max_username_length = 255 .signup-box.p-3.mb-2 .signup-body @@ -6,9 +6,6 @@ .devise-errors.mt-0 = render "devise/shared/error_messages", resource: resource = invisible_captcha - .name.form-group - = f.label :name, _('Full name'), class: 'label-bold' - = f.text_field :name, class: "form-control top js-block-emoji js-validate-length", :data => { :max_length => max_name_length, :max_length_message => s_("SignUp|Name is too long (maximum is %{max_length} characters).") % { max_length: max_name_length }, :qa_selector => 'new_user_name_field' }, required: true, title: _("This field is required.") .username.form-group = f.label :username, class: 'label-bold' = f.text_field :username, class: "form-control middle js-block-emoji js-validate-length js-validate-username", :data => { :max_length => max_username_length, :max_length_message => s_("SignUp|Username is too long (maximum is %{max_length} characters).") % { max_length: max_username_length }, :qa_selector => 'new_user_username_field' }, pattern: Gitlab::PathRegex::NAMESPACE_FORMAT_REGEX_JS, required: true, title: _("Please create a username with only alphanumeric characters.") diff --git a/app/views/devise/shared/_signin_box.html.haml b/app/views/devise/shared/_signin_box.html.haml index deaceccbfc7..746d43edbad 100644 --- a/app/views/devise/shared/_signin_box.html.haml +++ b/app/views/devise/shared/_signin_box.html.haml @@ -23,7 +23,7 @@ .login-body = render 'devise/sessions/new_base' -- if use_experimental_separate_sign_up_flow? +- if experiment_enabled?(:signup_flow) %p.light.mt-2 = _("Don't have an account yet?") = link_to _("Register now"), new_registration_path(:user) diff --git a/app/views/layouts/_head.html.haml b/app/views/layouts/_head.html.haml index 1efd8647a67..b8c9f0ae1e8 100644 --- a/app/views/layouts/_head.html.haml +++ b/app/views/layouts/_head.html.haml @@ -5,7 +5,7 @@ -# anything from this page beforehand. -# Part of an experiment to build a new sign up flow. Will be removed again with -# https://gitlab.com/gitlab-org/growth/engineering/issues/64 -- if use_experimental_separate_sign_up_flow? && current_path?("sessions#new") +- if experiment_enabled?(:signup_flow) && current_path?("sessions#new") = javascript_tag nonce: true do :plain if (window.location.hash === '#register-pane') { diff --git a/app/views/layouts/devise_experimental_separate_sign_up_flow.html.haml b/app/views/layouts/devise_experimental_separate_sign_up_flow.html.haml index b10145b62af..2f05717fc0e 100644 --- a/app/views/layouts/devise_experimental_separate_sign_up_flow.html.haml +++ b/app/views/layouts/devise_experimental_separate_sign_up_flow.html.haml @@ -14,7 +14,8 @@ = render_if_exists 'layouts/devise_help_text' .text-center.signup-heading.mt-3.mb-3 = image_tag(image_url('logo.svg'), class: 'gitlab-logo', alt: 'GitLab Logo') - %h2= _('Register for GitLab.com') + - if content_for?(:page_title) + %h2= yield :page_title = yield %hr.footer-fixed .footer-container diff --git a/app/views/profiles/show.html.haml b/app/views/profiles/show.html.haml index 0576f51fa83..68b7efc6fb4 100644 --- a/app/views/profiles/show.html.haml +++ b/app/views/profiles/show.html.haml @@ -94,6 +94,7 @@ - else = f.text_field :name, label: s_('Profiles|Full name'), required: true, title: s_("Profiles|Using emojis in names seems fun, but please try to set a status message instead"), wrapper: { class: 'col-md-9 qa-full-name rspec-full-name' }, help: s_("Profiles|Enter your name, so people you know can recognize you") = f.text_field :id, readonly: true, label: s_('Profiles|User ID'), wrapper: { class: 'col-md-3' } + = f.select :role, ::User.roles.keys.map { |role| [role.titleize, role] }, {}, class: 'input-md' = render_if_exists 'profiles/email_settings', form: f = f.text_field :skype, class: 'input-md', placeholder: s_("Profiles|username") diff --git a/app/views/registrations/welcome.html.haml b/app/views/registrations/welcome.html.haml new file mode 100644 index 00000000000..02ab974ecc0 --- /dev/null +++ b/app/views/registrations/welcome.html.haml @@ -0,0 +1,17 @@ +- content_for(:page_title, _('Welcome to GitLab<br>%{username}!' % { username: html_escape(current_user.username) }).html_safe) +- max_name_length = 128 +.text-center.mb-3 + = _('In order to tailor your experience with GitLab<br>we would like to know a bit more about you.').html_safe +.signup-box.p-3.mb-2 + .signup-body + = form_for(current_user, url: users_sign_up_update_role_path, html: { class: 'new_new_user gl-show-field-errors', 'aria-live' => 'assertive' }) do |f| + .devise-errors.mt-0 + = render 'devise/shared/error_messages', resource: current_user + .name.form-group + = f.label :name, _('Full name'), class: 'label-bold' + = f.text_field :name, class: 'form-control top js-block-emoji js-validate-length', :data => { :max_length => max_name_length, :max_length_message => s_('Name is too long (maximum is %{max_length} characters).') % { max_length: max_name_length }, :qa_selector => 'new_user_name_field' }, required: true, title: _('This field is required.') + .form-group + = f.label :role, _('Role'), class: 'label-bold' + = f.select :role, ::User.roles.keys.map { |role| [role.titleize, role] }, {}, class: 'form-control' + .submit-container.mt-3 + = f.submit _('Get started!'), class: 'btn-register btn btn-block mb-0 p-2' diff --git a/changelogs/unreleased/20-add-signup-step-2.yml b/changelogs/unreleased/20-add-signup-step-2.yml new file mode 100644 index 00000000000..cfaa57cea97 --- /dev/null +++ b/changelogs/unreleased/20-add-signup-step-2.yml @@ -0,0 +1,5 @@ +--- +title: Add step 2 of the experimental signup flow +merge_request: 16583 +author: +type: changed diff --git a/config/routes.rb b/config/routes.rb index f3dd43f78a9..5bfae777f17 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -55,6 +55,10 @@ Rails.application.routes.draw do get '/autocomplete/project_groups' => 'autocomplete#project_groups' end + # Sign up + get 'users/sign_up/welcome' => 'registrations#welcome' + patch 'users/sign_up/update_role' => 'registrations#update_role' + # Search get 'search' => 'search#show' get 'search/autocomplete' => 'search#autocomplete', as: :search_autocomplete diff --git a/db/migrate/20190912223232_add_role_to_users.rb b/db/migrate/20190912223232_add_role_to_users.rb new file mode 100644 index 00000000000..afbd78ed509 --- /dev/null +++ b/db/migrate/20190912223232_add_role_to_users.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddRoleToUsers < ActiveRecord::Migration[5.2] + DOWNTIME = false + + def change + add_column :users, :role, :smallint + end +end diff --git a/db/schema.rb b/db/schema.rb index b39eb408b0a..f3a2b4608f5 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -3780,6 +3780,7 @@ ActiveRecord::Schema.define(version: 2019_10_16_220135) do t.string "first_name", limit: 255 t.string "last_name", limit: 255 t.string "static_object_token", limit: 255 + t.integer "role", limit: 2 t.index "lower((name)::text)", name: "index_on_users_name_lower" t.index ["accepted_term_id"], name: "index_users_on_accepted_term_id" t.index ["admin"], name: "index_users_on_admin" diff --git a/doc/administration/high_availability/README.md b/doc/administration/high_availability/README.md index 3f842d51d5e..199944a160c 100644 --- a/doc/administration/high_availability/README.md +++ b/doc/administration/high_availability/README.md @@ -234,14 +234,21 @@ future based on additional testing and iteration. - **Supported Users (approximate):** 25,000 - **RPS:** 500 requests per second -- **Status:** Work-in-progress -- **Related Issue:** See the [related issue](https://gitlab.com/gitlab-org/quality/performance/issues/57) for more information. +- **Known Issues:** The slow API endpoints that were discovered during testing + the 10,000 user architecture also affect the 25,000 user architecture. For + details, see the related issues list in + [this issue](https://gitlab.com/gitlab-org/gitlab-foss/issues/64335). -The Support and Quality teams are in the process of building and performance -testing an environment that will support around 25,000 users. The specifications -below are a work-in-progress representation of the work so far. The Quality team -will be certifying this environment in late 2019. The specifications may be -adjusted prior to certification based on performance testing. +The GitLab Support and Quality teams built, performance tested, and validated an +environment that supports around 25,000 users. The specifications below are a +representation of the work so far. The specifications may be adjusted in the +future based on additional testing and iteration. + +NOTE: **Note:** The specifications here were performance tested against a +specific coded workload. Your exact needs may be more, depending on your +workload. Your workload is influenced by factors such as - but not limited to - +how active your users are, how much automation you use, mirroring, and +repo/change size. | Service | Configuration | GCP type | | ------------------------------|-------------------------|----------------| diff --git a/lib/gitlab/experimentation.rb b/lib/gitlab/experimentation.rb index 678d47150e8..895755376ee 100644 --- a/lib/gitlab/experimentation.rb +++ b/lib/gitlab/experimentation.rb @@ -40,8 +40,8 @@ module Gitlab } end - def experiment_enabled?(experiment) - Experimentation.enabled?(experiment, experimentation_subject_index) + def experiment_enabled?(experiment_key) + Experimentation.enabled?(experiment_key, experimentation_subject_index) end private @@ -55,10 +55,14 @@ module Gitlab end class << self + def experiment(key) + Experiment.new(EXPERIMENTS[key].merge(key: key)) + end + def enabled?(experiment_key, experimentation_subject_index) return false unless EXPERIMENTS.key?(experiment_key) - experiment = Experiment.new(EXPERIMENTS[experiment_key].merge(key: experiment_key)) + experiment = experiment(experiment_key) experiment.feature_toggle_enabled? && experiment.enabled_for_environment? && diff --git a/locale/gitlab.pot b/locale/gitlab.pot index bf24381c031..b56d50cabad 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -7841,6 +7841,9 @@ msgstr "" msgid "Get started with performance monitoring" msgstr "" +msgid "Get started!" +msgstr "" + msgid "Getting started with releases" msgstr "" @@ -8917,6 +8920,9 @@ msgstr "" msgid "In order to gather accurate feature usage data, it can take 1 to 2 weeks to see your index." msgstr "" +msgid "In order to tailor your experience with GitLab<br>we would like to know a bit more about you." +msgstr "" + msgid "In the next step, you'll be able to select the projects you want to import." msgstr "" @@ -10708,6 +10714,9 @@ msgstr "" msgid "Name has already been taken" msgstr "" +msgid "Name is too long (maximum is %{max_length} characters)." +msgstr "" + msgid "Name new label" msgstr "" @@ -13513,7 +13522,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" @@ -14092,6 +14101,9 @@ msgstr "" msgid "Roadmap" msgstr "" +msgid "Role" +msgstr "" + msgid "Rollback" msgstr "" diff --git a/package.json b/package.json index 54f803a8964..0d951a58406 100644 --- a/package.json +++ b/package.json @@ -38,7 +38,7 @@ "@babel/plugin-syntax-import-meta": "^7.2.0", "@babel/preset-env": "^7.6.2", "@gitlab/svgs": "^1.78.0", - "@gitlab/ui": "5.35.0", + "@gitlab/ui": "5.36.0", "@gitlab/visual-review-tools": "1.0.3", "apollo-cache-inmemory": "^1.5.1", "apollo-client": "^2.5.1", diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 5e33421854b..ed91b5973b8 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -842,4 +842,48 @@ describe ApplicationController do end end end + + describe '#require_role' do + controller(described_class) do + def index; end + end + + let(:user) { create(:user) } + let(:experiment_enabled) { true } + + before do + stub_experiment(signup_flow: experiment_enabled) + end + + 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 'experiment enabled and user without a role' do + before do + sign_in(user) + get :index + end + + it { is_expected.not_to redirect_to users_sign_up_welcome_path } + end + + context 'experiment disabled and user with required role' do + let(:experiment_enabled) { false } + + before do + user.set_role_required! + sign_in(user) + get :index + end + + it { is_expected.not_to redirect_to users_sign_up_welcome_path } + end + end end diff --git a/spec/controllers/registrations_controller_spec.rb b/spec/controllers/registrations_controller_spec.rb index 5d87dbdee8b..ebeed94c274 100644 --- a/spec/controllers/registrations_controller_spec.rb +++ b/spec/controllers/registrations_controller_spec.rb @@ -114,9 +114,14 @@ describe RegistrationsController do context 'when invisible captcha is enabled' do before do stub_feature_flags(invisible_captcha: true) + InvisibleCaptcha.timestamp_enabled = true InvisibleCaptcha.timestamp_threshold = treshold end + after do + InvisibleCaptcha.timestamp_enabled = false + end + let(:treshold) { 4 } let(:session_params) { { invisible_captcha_timestamp: form_rendered_time.iso8601 } } let(:form_rendered_time) { Time.current } diff --git a/spec/factories/users.rb b/spec/factories/users.rb index e3d20c1f46c..f83c137b758 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -6,6 +6,7 @@ FactoryBot.define do name { generate(:name) } username { generate(:username) } password { "12345678" } + role { 'software_developer' } confirmed_at { Time.now } confirmation_token { nil } can_create_group { true } diff --git a/spec/features/invites_spec.rb b/spec/features/invites_spec.rb index 1e054a7b358..2a1980346e9 100644 --- a/spec/features/invites_spec.rb +++ b/spec/features/invites_spec.rb @@ -10,7 +10,6 @@ describe 'Invites' do let(:group_invite) { group.group_members.invite.last } before do - stub_feature_flags(invisible_captcha: false) project.add_maintainer(owner) group.add_user(owner, Gitlab::Access::OWNER) group.add_developer('user@example.com', owner) diff --git a/spec/features/users/signup_spec.rb b/spec/features/users/signup_spec.rb index 0846ec8dfb4..562d6fcab1b 100644 --- a/spec/features/users/signup_spec.rb +++ b/spec/features/users/signup_spec.rb @@ -5,10 +5,6 @@ require 'spec_helper' shared_examples 'Signup' do include TermsHelper - before do - stub_feature_flags(invisible_captcha: false) - end - let(:new_user) { build_stubbed(:user) } describe 'username validation', :js do @@ -129,35 +125,43 @@ shared_examples 'Signup' do describe 'user\'s full name validation', :js do before do - visit new_user_registration_path + if Gitlab::Experimentation.enabled?(:signup_flow) + user = create(:user, role: nil) + sign_in(user) + visit users_sign_up_welcome_path + @user_name_field = 'user_name' + else + visit new_user_registration_path + @user_name_field = 'new_user_name' + end end it 'does not show an error border if the user\'s fullname length is not longer than 128 characters' do - fill_in 'new_user_name', with: 'u' * 128 + fill_in @user_name_field, with: 'u' * 128 expect(find('.name')).not_to have_css '.gl-field-error-outline' end it 'shows an error border if the user\'s fullname contains an emoji' do - simulate_input('#new_user_name', 'Ehsan 🦋') + simulate_input("##{@user_name_field}", 'Ehsan 🦋') expect(find('.name')).to have_css '.gl-field-error-outline' end it 'shows an error border if the user\'s fullname is longer than 128 characters' do - fill_in 'new_user_name', with: 'n' * 129 + fill_in @user_name_field, with: 'n' * 129 expect(find('.name')).to have_css '.gl-field-error-outline' end it 'shows an error message if the user\'s fullname is longer than 128 characters' do - fill_in 'new_user_name', with: 'n' * 129 + fill_in @user_name_field, with: 'n' * 129 expect(page).to have_content("Name is too long (maximum is 128 characters).") end it 'shows an error message if the username contains emojis' do - simulate_input('#new_user_name', 'Ehsan 🦋') + simulate_input("##{@user_name_field}", 'Ehsan 🦋') expect(page).to have_content("Invalid input, please avoid emojis") end @@ -177,11 +181,11 @@ shared_examples 'Signup' do it 'creates the user account and sends a confirmation email' do visit new_user_registration_path - fill_in 'new_user_name', with: new_user.name fill_in 'new_user_username', with: new_user.username fill_in 'new_user_email', with: new_user.email - unless Feature.enabled?(:experimental_separate_sign_up_flow) + unless Gitlab::Experimentation.enabled?(:signup_flow) + fill_in 'new_user_name', with: new_user.name fill_in 'new_user_email_confirmation', with: new_user.email end @@ -202,11 +206,11 @@ shared_examples 'Signup' do it 'creates the user account and sends a confirmation email' do visit new_user_registration_path - fill_in 'new_user_name', with: new_user.name fill_in 'new_user_username', with: new_user.username fill_in 'new_user_email', with: new_user.email - unless Feature.enabled?(:experimental_separate_sign_up_flow) + unless Gitlab::Experimentation.enabled?(:signup_flow) + fill_in 'new_user_name', with: new_user.name fill_in 'new_user_email_confirmation', with: new_user.email end @@ -214,8 +218,12 @@ shared_examples 'Signup' do expect { click_button 'Register' }.to change { User.count }.by(1) - expect(current_path).to eq dashboard_projects_path - expect(page).to have_content("Please check your email (#{new_user.email}) to verify that you own this address.") + if Gitlab::Experimentation.enabled?(:signup_flow) + expect(current_path).to eq users_sign_up_welcome_path + else + expect(current_path).to eq dashboard_projects_path + expect(page).to have_content("Please check your email (#{new_user.email}) to verify that you own this address.") + end end end end @@ -224,19 +232,23 @@ shared_examples 'Signup' do it "creates the user successfully" do visit new_user_registration_path - fill_in 'new_user_name', with: new_user.name fill_in 'new_user_username', with: new_user.username fill_in 'new_user_email', with: new_user.email - unless Feature.enabled?(:experimental_separate_sign_up_flow) + unless Gitlab::Experimentation.enabled?(:signup_flow) + fill_in 'new_user_name', with: new_user.name fill_in 'new_user_email_confirmation', with: new_user.email.capitalize end fill_in 'new_user_password', with: new_user.password click_button "Register" - expect(current_path).to eq dashboard_projects_path - expect(page).to have_content("Welcome! You have signed up successfully.") + if Gitlab::Experimentation.enabled?(:signup_flow) + expect(current_path).to eq users_sign_up_welcome_path + else + expect(current_path).to eq dashboard_projects_path + expect(page).to have_content("Welcome! You have signed up successfully.") + end end end @@ -248,19 +260,23 @@ shared_examples 'Signup' do it 'creates the user account and goes to dashboard' do visit new_user_registration_path - fill_in 'new_user_name', with: new_user.name fill_in 'new_user_username', with: new_user.username fill_in 'new_user_email', with: new_user.email - unless Feature.enabled?(:experimental_separate_sign_up_flow) + unless Gitlab::Experimentation.enabled?(:signup_flow) + fill_in 'new_user_name', with: new_user.name fill_in 'new_user_email_confirmation', with: new_user.email end fill_in 'new_user_password', with: new_user.password click_button "Register" - expect(current_path).to eq dashboard_projects_path - expect(page).to have_content("Welcome! You have signed up successfully.") + if Gitlab::Experimentation.enabled?(:signup_flow) + expect(current_path).to eq users_sign_up_welcome_path + else + expect(current_path).to eq dashboard_projects_path + expect(page).to have_content("Welcome! You have signed up successfully.") + end end end end @@ -271,7 +287,10 @@ shared_examples 'Signup' do visit new_user_registration_path - fill_in 'new_user_name', with: new_user.name + unless Gitlab::Experimentation.enabled?(:signup_flow) + fill_in 'new_user_name', with: new_user.name + end + fill_in 'new_user_username', with: new_user.username fill_in 'new_user_email', with: existing_user.email fill_in 'new_user_password', with: new_user.password @@ -279,14 +298,14 @@ shared_examples 'Signup' do expect(current_path).to eq user_registration_path - if Feature.enabled?(:experimental_separate_sign_up_flow) + if Gitlab::Experimentation.enabled?(:signup_flow) expect(page).to have_content("error prohibited this user from being saved") - expect(page).to have_content("Email has already been taken") else expect(page).to have_content("errors prohibited this user from being saved") - expect(page).to have_content("Email has already been taken") expect(page).to have_content("Email confirmation doesn't match") end + + expect(page).to have_content("Email has already been taken") end it 'does not redisplay the password' do @@ -294,7 +313,10 @@ shared_examples 'Signup' do visit new_user_registration_path - fill_in 'new_user_name', with: new_user.name + unless Gitlab::Experimentation.enabled?(:signup_flow) + fill_in 'new_user_name', with: new_user.name + end + fill_in 'new_user_username', with: new_user.username fill_in 'new_user_email', with: existing_user.email fill_in 'new_user_password', with: new_user.password @@ -313,11 +335,11 @@ shared_examples 'Signup' do it 'requires the user to check the checkbox' do visit new_user_registration_path - fill_in 'new_user_name', with: new_user.name fill_in 'new_user_username', with: new_user.username fill_in 'new_user_email', with: new_user.email - unless Feature.enabled?(:experimental_separate_sign_up_flow) + unless Gitlab::Experimentation.enabled?(:signup_flow) + fill_in 'new_user_name', with: new_user.name fill_in 'new_user_email_confirmation', with: new_user.email end @@ -332,11 +354,11 @@ shared_examples 'Signup' do it 'asks the user to accept terms before going to the dashboard' do visit new_user_registration_path - fill_in 'new_user_name', with: new_user.name fill_in 'new_user_username', with: new_user.username fill_in 'new_user_email', with: new_user.email - unless Feature.enabled?(:experimental_separate_sign_up_flow) + unless Gitlab::Experimentation.enabled?(:signup_flow) + fill_in 'new_user_name', with: new_user.name fill_in 'new_user_email_confirmation', with: new_user.email end @@ -345,24 +367,84 @@ shared_examples 'Signup' do click_button "Register" - expect(current_path).to eq dashboard_projects_path + if Gitlab::Experimentation.enabled?(:signup_flow) + expect(current_path).to eq users_sign_up_welcome_path + else + expect(current_path).to eq dashboard_projects_path + end end end -end -describe 'With original flow' do - it_behaves_like 'Signup' do + context 'when reCAPTCHA and invisible captcha are enabled' do before do - stub_feature_flags(experimental_separate_sign_up_flow: false) + InvisibleCaptcha.timestamp_enabled = true + stub_application_setting(recaptcha_enabled: true) + allow_any_instance_of(RegistrationsController).to receive(:verify_recaptcha).and_return(false) + end + + after do + InvisibleCaptcha.timestamp_enabled = false + end + + it 'prevents from signing up' do + visit new_user_registration_path + + fill_in 'new_user_username', with: new_user.username + fill_in 'new_user_email', with: new_user.email + + unless Gitlab::Experimentation.enabled?(:signup_flow) + fill_in 'new_user_name', with: new_user.name + fill_in 'new_user_email_confirmation', with: new_user.email + end + + fill_in 'new_user_password', with: new_user.password + + expect { click_button 'Register' }.not_to change { User.count } + + if Gitlab::Experimentation.enabled?(:signup_flow) + expect(page).to have_content('That was a bit too quick! Please resubmit.') + else + expect(page).to have_content('There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.') + end end end end -describe 'With experimental flow on GitLab.com' do - it_behaves_like 'Signup' do - before do - expect(Gitlab).to receive(:com?).and_return(true).at_least(:once) - stub_feature_flags(experimental_separate_sign_up_flow: true) +describe 'With original flow' do + before do + stub_experiment(signup_flow: false) + end + + it_behaves_like 'Signup' +end + +describe 'With experimental flow' do + before do + stub_experiment(signup_flow: true) + end + + it_behaves_like 'Signup' + + describe 'when role is required' do + it 'after registering, it redirects to step 2 of the signup process, sets the name and role and then redirects to the original requested url' do + new_user = build_stubbed(:user) + visit new_user_registration_path + fill_in 'new_user_username', with: new_user.username + fill_in 'new_user_email', with: new_user.email + fill_in 'new_user_password', with: new_user.password + click_button 'Register' + visit new_project_path + + expect(page).to have_current_path(users_sign_up_welcome_path) + + fill_in 'user_name', with: 'New name' + select 'Software Developer', from: 'user_role' + click_button 'Get started!' + new_user = User.find_by_username(new_user.username) + + expect(new_user.name).to eq 'New name' + expect(new_user.software_developer_role?).to be_truthy + expect(page).to have_current_path(new_project_path) end end end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 0d190ae069e..ee4e783e9ac 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -634,40 +634,21 @@ describe API::Users do end describe "GET /users/sign_up" do - context 'when experimental_separate_sign_up_flow is active' do + context 'when experimental signup_flow is active' do before do - stub_feature_flags(experimental_separate_sign_up_flow: true) + stub_experiment(signup_flow: true) end - context 'on gitlab.com' do - before do - allow(::Gitlab).to receive(:com?).and_return(true) - end - - it "shows sign up page" do - get "/users/sign_up" - expect(response).to have_gitlab_http_status(200) - expect(response).to render_template(:new) - end - end - - context 'not on gitlab.com' do - before do - allow(::Gitlab).to receive(:com?).and_return(false) - end - - it "redirects to sign in page" do - get "/users/sign_up" - expect(response).to have_gitlab_http_status(302) - expect(response).to redirect_to(new_user_session_path(anchor: 'register-pane')) - end + it "shows sign up page" do + get "/users/sign_up" + expect(response).to have_gitlab_http_status(200) + expect(response).to render_template(:new) end end - context 'when experimental_separate_sign_up_flow is not active' do + context 'when experimental signup_flow is not active' do before do - allow(::Gitlab).to receive(:com?).and_return(true) - stub_feature_flags(experimental_separate_sign_up_flow: false) + stub_experiment(signup_flow: false) end it "redirects to sign in page" do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 61e50fe5723..7a5e570558e 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -88,6 +88,7 @@ RSpec.configure do |config| config.include FixtureHelpers config.include GitlabRoutingHelper config.include StubFeatureFlags + config.include StubExperiments config.include StubGitlabCalls config.include StubGitlabData config.include NextInstanceOf @@ -378,3 +379,6 @@ end # Prevent Rugged from picking up local developer gitconfig. Rugged::Settings['search_path_global'] = Rails.root.join('tmp/tests').to_s + +# Disable timestamp checks for invisible_captcha +InvisibleCaptcha.timestamp_enabled = false diff --git a/spec/support/helpers/stub_experiments.rb b/spec/support/helpers/stub_experiments.rb new file mode 100644 index 00000000000..ed868e22c6e --- /dev/null +++ b/spec/support/helpers/stub_experiments.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module StubExperiments + # Stub Experiment with `key: true/false` + # + # @param [Hash] experiment where key is feature name and value is boolean whether enabled or not. + # + # Examples + # - `stub_experiment(signup_flow: false)` ... Disable `signup_flow` experiment globally. + def stub_experiment(experiments) + experiments.each do |experiment_key, enabled| + allow(Gitlab::Experimentation).to receive(:enabled?).with(experiment_key, any_args) { enabled } + end + end +end diff --git a/spec/views/devise/shared/_signin_box.html.haml_spec.rb b/spec/views/devise/shared/_signin_box.html.haml_spec.rb index 6d640686337..f8867477603 100644 --- a/spec/views/devise/shared/_signin_box.html.haml_spec.rb +++ b/spec/views/devise/shared/_signin_box.html.haml_spec.rb @@ -10,6 +10,7 @@ describe 'devise/shared/_signin_box' do allow(view).to receive(:current_application_settings).and_return(Gitlab::CurrentSettings.current_application_settings) allow(view).to receive(:captcha_enabled?).and_return(false) allow(view).to receive(:captcha_on_login_required?).and_return(false) + allow(view).to receive(:experiment_enabled?).and_return(false) end it 'is shown when Crowd is enabled' do diff --git a/spec/views/layouts/_head.html.haml_spec.rb b/spec/views/layouts/_head.html.haml_spec.rb index bea0b0edf4d..e9b3334fffc 100644 --- a/spec/views/layouts/_head.html.haml_spec.rb +++ b/spec/views/layouts/_head.html.haml_spec.rb @@ -7,6 +7,7 @@ describe 'layouts/_head' do before do allow(view).to receive(:current_application_settings).and_return(Gitlab::CurrentSettings.current_application_settings) + allow(view).to receive(:experiment_enabled?).and_return(false) end it 'escapes HTML-safe strings in page_title' do diff --git a/yarn.lock b/yarn.lock index fbaddce43c6..3abd18d1111 100644 --- a/yarn.lock +++ b/yarn.lock @@ -995,10 +995,10 @@ resolved "https://registry.yarnpkg.com/@gitlab/svgs/-/svgs-1.78.0.tgz#469493bd6cdd254eb5d1271edeab22bbbee2f4c4" integrity sha512-dBgEB/Q4FRD0NapmNrD86DF1FsV0uSgTx0UOJloHnGE2DNR2P1HQrCmLW2fX+QgN4P9CDAzdi2buVHuholofWw== -"@gitlab/ui@5.35.0": - version "5.35.0" - resolved "https://registry.yarnpkg.com/@gitlab/ui/-/ui-5.35.0.tgz#843e9febf1d4ef9b846dc3280e32e3e626c6f9b1" - integrity sha512-PD9hqVlRhwYRPbL+u/gcHew8NfPXbphZ0CQqfIXaWUYdEOMksUtP6DnLToG6S321WjrCMD+IBHnVQxf2juZBxg== +"@gitlab/ui@5.36.0": + version "5.36.0" + resolved "https://registry.yarnpkg.com/@gitlab/ui/-/ui-5.36.0.tgz#3087b23c138ad1c222f6b047e533f253371bc618" + integrity sha512-XXWUYZbRItKh9N92Vxql04BJ05uW5HlOuTCkD+lMbUgneqYTgVoKGH8d9kD++Jy7q8l5+AfzjboUn2n9sbQMZA== dependencies: "@babel/standalone" "^7.0.0" "@gitlab/vue-toasted" "^1.2.1" |