More feedback fixes

- A slightly bigger margin in a view
- Fixed test race condition
- Comment rewording
- Remove migration helper from migration
- Disable reCAPTCHA when signup flow is enabled
- HTML escape username
- Expanded issue URL in comment
- Add changelog

app/controllers/application_controller.rb

@@ -549,9 +549,8 @@ class ApplicationController < ActionController::Base
     @current_user_mode ||= Gitlab::Auth::CurrentUserMode.new(current_user)
   end
 
-  # A user requires a role when he is part of the experimental signup flow (executed by the Growth team)
-  # 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.
+  # 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)
 

app/controllers/registrations_controller.rb

@@ -136,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!
 

app/models/user.rb

@@ -1563,7 +1563,8 @@ class User < ApplicationRecord
   end
 
   # Below is used for the signup_flow experiment. Should be removed
-  # when experiment finishes. See gitlab-org/growth/engineering#64
+  # when experiment finishes.
+  # See https://gitlab.com/gitlab-org/growth/engineering/issues/64
   REQUIRES_ROLE_VALUE = 99
 
   def role_required?

app/views/registrations/welcome.html.haml

-- content_for(:page_title, _('Welcome to GitLab<br>%{username}!' % { username: current_user.username }).html_safe)
+- content_for(:page_title, _('Welcome to GitLab<br>%{username}!' % { username: html_escape(current_user.username) }).html_safe)
 - max_name_length = 128
-.text-center.mb-2
+.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

changelogs/unreleased/20-add-signup-step-2.yml

+---
+title: Add step 2 of the experimental signup flow
+merge_request: 16583
+author:
+type: changed

db/migrate/20190912223232_add_role_to_users.rb

@@ -4,8 +4,6 @@
 # for more information on how to write migrations for GitLab.
 
 class AddRoleToUsers < ActiveRecord::Migration[5.2]
-  include Gitlab::Database::MigrationHelpers
-
   DOWNTIME = false
 
   def change

spec/features/users/signup_spec.rb

@@ -374,6 +374,40 @@ shared_examples 'Signup' do
       end
     end
   end
+
+  context 'when reCAPTCHA and invisible captcha are enabled' do
+    before do
+      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 original flow' do
@@ -392,21 +426,24 @@ describe 'With experimental flow' do
   it_behaves_like 'Signup'
 
   describe 'when role is required' do
-    it 'redirects to step 2 of the signup process, updates the user, sets the name and role and then redirects to the requested url' do
-      user = create(:user)
-      user.set_role_required!
-      sign_in(user)
+    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(current_path).to eq users_sign_up_welcome_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)
 
-      user.reload
-      expect(user.name).to eq 'New name'
-      expect(user.software_developer_role?).to be_truthy
+      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