Implement feedback fixes

- Simplify signup service
- Retain submitted form information on error
- Show full name when it has been filled in already
- Move setup_for_company attribute to the
user_preferences table
- Used sass variable for padding
- Updated specs to Four-Phase Test Pattern
- Explicitly set layout
- Fix redirect logic
- Display all possible error messages

app/assets/stylesheets/pages/experimental_separate_sign_up.scss

@@ -23,7 +23,7 @@
 
   .signup-heading h2 {
     font-weight: $gl-font-weight-bold;
-    padding: 0 10px;
+    padding: 0 $gl-padding;
 
     @include media-breakpoint-down(md) {
       font-size: $gl-font-size-large;

app/controllers/application_controller.rb

@@ -541,7 +541,7 @@ class ApplicationController < ActionController::Base
   # experiment is enabled for the current user.
   def required_signup_info
     return unless current_user
-    return unless current_user.role_required? || current_user.setup_for_company.nil?
+    return unless current_user.role_required?
     return unless experiment_enabled?(:signup_flow)
 
     store_location_for :user, request.fullpath

app/controllers/registrations_controller.rb

@@ -53,22 +53,22 @@ class RegistrationsController < Devise::RegistrationsController
 
   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.setup_for_company.present?
+    return redirect_to stored_location_or_dashboard_or_almost_there_path(current_user) if current_user.role.present? && !current_user.setup_for_company.nil?
 
-    current_user.name = nil
+    current_user.name = nil if current_user.name == current_user.username
     render layout: 'devise_experimental_separate_sign_up_flow'
   end
 
   def update_registration
     user_params = params.require(:user).permit(:name, :role, :setup_for_company)
-    result = ::Users::SignupService.new(current_user, user_params.merge(user: current_user)).execute
+    result = ::Users::SignupService.new(current_user, user_params).execute
 
     if result[:status] == :success
       track_experiment_event(:signup_flow, 'end') # We want this event to be tracked when the user is _in_ the experimental group
       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]
+      render :welcome, layout: 'devise_experimental_separate_sign_up_flow'
     end
   end
 

app/models/user.rb

@@ -240,6 +240,7 @@ class User < ApplicationRecord
   delegate :time_display_relative, :time_display_relative=, to: :user_preference
   delegate :time_format_in_24h, :time_format_in_24h=, to: :user_preference
   delegate :show_whitespace_in_diffs, :show_whitespace_in_diffs=, to: :user_preference
+  delegate :setup_for_company, :setup_for_company=, to: :user_preference
 
   accepts_nested_attributes_for :user_preference, update_only: true
 

app/services/users/signup_service.rb

@@ -2,25 +2,19 @@
 
 module Users
   class SignupService < BaseService
-    attr_reader :user
-
     def initialize(current_user, params = {})
-      @current_user = current_user
-      @user = params.delete(:user)
+      @user = current_user
       @params = params.dup
     end
 
-    def execute(validate: true, &block)
-      yield(@user) if block_given?
-
+    def execute
       assign_attributes
-      custom_validations
+      inject_validators
 
-      if @user.errors.empty? && @user.save(validate: validate)
+      if @user.save
         success
       else
-        messages = @user.errors.full_messages + Array(@user.status&.errors&.full_messages)
-        error(messages.uniq.join('. '))
+        error(@user.errors.full_messages.join('. '))
       end
     end
 
@@ -30,10 +24,11 @@ module Users
       @user.assign_attributes(params) unless params.empty?
     end
 
-    def custom_validations
-      @user.errors.add(:base, 'Please fill in your full name') if @user.name.blank?
-      @user.errors.add(:base, 'Please select your role') if @user.role.blank?
-      @user.errors.add(:base, 'Please answer "Are you setting up GitLab for a company?"') if @user.setup_for_company.nil?
+    def inject_validators
+      class << @user
+        validates :role, presence: true
+        validates :setup_for_company, inclusion: { in: [true, false], message: :blank }
+      end
     end
   end
 end

app/views/registrations/welcome.html.haml

-- content_for(:page_title, _('Welcome to GitLab %{username}!') % { username: current_user.username })
+- content_for(:page_title, _('Welcome to GitLab @%{username}!') % { username: current_user.username })
 - 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
+  = _('In order to tailor your experience with GitLab we<br>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_registration_path, html: { class: 'new_new_user gl-show-field-errors', 'aria-live' => 'assertive' }) do |f|

db/migrate/20191002124613_add_setup_for_company_to_users.rb → db/migrate/20191028162543_add_setup_for_company_to_user_preferences.rb

 # frozen_string_literal: true
 
-class AddSetupForCompanyToUsers < ActiveRecord::Migration[5.2]
+class AddSetupForCompanyToUserPreferences < ActiveRecord::Migration[5.2]
   DOWNTIME = false
 
   def change
-    add_column :users, :setup_for_company, :boolean
+    add_column :user_preferences, :setup_for_company, :boolean
   end
 end

db/schema.rb

@@ -3754,6 +3754,7 @@ ActiveRecord::Schema.define(version: 2019_11_05_094625) do
     t.boolean "time_format_in_24h"
     t.string "projects_sort", limit: 64
     t.boolean "show_whitespace_in_diffs", default: true, null: false
+    t.boolean "setup_for_company"
     t.index ["user_id"], name: "index_user_preferences_on_user_id", unique: true
   end
 
@@ -3858,7 +3859,6 @@ ActiveRecord::Schema.define(version: 2019_11_05_094625) do
     t.string "last_name", limit: 255
     t.string "static_object_token", limit: 255
     t.integer "role", limit: 2
-    t.boolean "setup_for_company"
     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"

locale/gitlab.pot

@@ -9146,7 +9146,7 @@ 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."
+msgid "In order to tailor your experience with GitLab we<br>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."
@@ -19093,7 +19093,7 @@ msgstr ""
 msgid "Welcome to GitLab"
 msgstr ""
 
-msgid "Welcome to GitLab %{username}!"
+msgid "Welcome to GitLab @%{username}!"
 msgstr ""
 
 msgid "Welcome to the Guided GitLab Tour"

spec/controllers/application_controller_spec.rb

@@ -832,9 +832,8 @@ describe ApplicationController do
       def index; end
     end
 
-    let(:user) { create(:user, setup_for_company: setup_for_company) }
+    let(:user) { create(:user) }
     let(:experiment_enabled) { true }
-    let(:setup_for_company) { true }
 
     before do
       stub_experiment_for_user(signup_flow: experiment_enabled)
@@ -850,18 +849,7 @@ describe ApplicationController do
       it { is_expected.to redirect_to users_sign_up_welcome_path }
     end
 
-    context 'experiment enabled and user without the setup_for_company attribute set' do
-      let(:setup_for_company) { nil }
-
-      before do
-        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 required role and with the setup_for_company attribute set' do
+    context 'experiment enabled and user without a required role' do
       before do
         sign_in(user)
         get :index
@@ -872,7 +860,6 @@ describe ApplicationController do
 
     context 'experiment disabled' do
       let(:experiment_enabled) { false }
-      let(:setup_for_company) { nil }
 
       before do
         user.set_role_required!

spec/features/users/signup_spec.rb

@@ -441,12 +441,13 @@ describe 'With experimental flow' do
 
       fill_in 'user_name', with: 'New name'
       select 'Software Developer', from: 'user_role'
-      choose 'user_setup_for_company_false'
+      choose 'user_setup_for_company_true'
       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(new_user.setup_for_company).to be_truthy
       expect(page).to have_current_path(new_project_path)
     end
   end

spec/services/users/signup_service_spec.rb

@@ -6,6 +6,7 @@ describe Users::SignupService do
   let(:user) { create(:user, setup_for_company: true) }
 
   describe '#execute' do
+    context 'when updating name' do
       it 'updates the name attribute' do
         result = update_user(user, name: 'New Name')
 
@@ -13,6 +14,16 @@ describe Users::SignupService do
         expect(user.reload.name).to eq('New Name')
       end
 
+      it 'returns an error result when name is missing' do
+        result = update_user(user, name: '')
+
+        expect(user.reload.name).not_to be_blank
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to include("Name can't be blank")
+      end
+    end
+
+    context 'when updating role' do
       it 'updates the role attribute' do
         result = update_user(user, role: 'development_team_lead')
 
@@ -20,42 +31,34 @@ describe Users::SignupService do
         expect(user.reload.role).to eq('development_team_lead')
       end
 
+      it 'returns an error result when role is missing' do
+        result = update_user(user, role: '')
+
+        expect(user.reload.role).not_to be_blank
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq("Role can't be blank")
+      end
+    end
+
+    context 'when updating setup_for_company' do
       it 'updates the setup_for_company attribute' do
         result = update_user(user, setup_for_company: 'false')
 
         expect(result).to eq(status: :success)
-      expect(user.reload.setup_for_company).to be_falsey
+        expect(user.reload.setup_for_company).to be(false)
       end
 
-    it 'returns an error result when name is missing' do
-      result = {}
-      expect do
-        result = update_user(user, { name: '' })
-      end.not_to change { user.reload.name }
-      expect(result[:status]).to eq(:error)
-      expect(result[:message]).to eq('Please fill in your full name')
-    end
+      it 'returns an error result when setup_for_company is missing' do
+        result = update_user(user, setup_for_company: '')
 
-    it 'returns an error result when role is missing' do
-      result = {}
-      expect do
-        result = update_user(user, { role: '' })
-      end.not_to change { user.reload.role }
+        expect(user.reload.setup_for_company).not_to be_blank
         expect(result[:status]).to eq(:error)
-      expect(result[:message]).to eq('Please select your role')
+        expect(result[:message]).to eq("Setup for company can't be blank")
       end
-
-    it 'returns an error result when setup_for_company is missing' do
-      result = {}
-      expect do
-        result = update_user(user, { setup_for_company: '' })
-      end.not_to change { user.reload.setup_for_company }
-      expect(result[:status]).to eq(:error)
-      expect(result[:message]).to eq('Please answer "Are you setting up GitLab for a company?"')
     end
 
     def update_user(user, opts)
-      described_class.new(user, opts.merge(user: user)).execute
+      described_class.new(user, opts).execute
     end
   end
 end