Merge branch 'nicolasdular/split-signup-full-name' into 'master'

Use first and last name for new sign up flow

See merge request gitlab-org/gitlab!22840

app/controllers/registrations_controller.rb

@@ -60,7 +60,7 @@ class RegistrationsController < Devise::RegistrationsController
   end
 
   def update_registration
-    user_params = params.require(:user).permit(:name, :role, :setup_for_company)
+    user_params = params.require(:user).permit(:role, :setup_for_company)
     result = ::Users::SignupService.new(current_user, user_params).execute
 
     if result[:status] == :success
@@ -152,13 +152,7 @@ class RegistrationsController < Devise::RegistrationsController
   end
 
   def sign_up_params
-    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
+    params.require(:user).permit(:username, :email, :email_confirmation, :name, :first_name, :last_name, :password)
   end
 
   def resource_name

app/models/user.rb

@@ -164,9 +164,9 @@ class User < ApplicationRecord
   # Validations
   #
   # Note: devise :validatable above adds validations for :email and :password
-  validates :name, presence: true, length: { maximum: 128 }
-  validates :first_name, length: { maximum: 255 }
-  validates :last_name, length: { maximum: 255 }
+  validates :name, presence: true, length: { maximum: 255 }
+  validates :first_name, length: { maximum: 127 }
+  validates :last_name, length: { maximum: 127 }
   validates :email, confirmation: true
   validates :notification_email, presence: true
   validates :notification_email, devise_email: true, if: ->(user) { user.notification_email != user.email }

app/services/users/build_service.rb

@@ -86,6 +86,8 @@ module Users
         :email_confirmation,
         :password_automatically_set,
         :name,
+        :first_name,
+        :last_name,
         :password,
         :username
       ]
@@ -107,6 +109,12 @@ module Users
         if user_params[:skip_confirmation].nil?
           user_params[:skip_confirmation] = skip_user_confirmation_email_from_setting
         end
+
+        fallback_name = "#{user_params[:first_name]} #{user_params[:last_name]}"
+
+        if user_params[:name].blank? && fallback_name.present?
+          user_params = user_params.merge(name: fallback_name)
+        end
       end
 
       if user_default_internal_regex_enabled? && !user_params.key?(:external)

app/views/devise/shared/_experimental_separate_sign_up_flow_box.html.haml

 - content_for(:page_title, _('Register for GitLab'))
+- max_first_name_length = max_last_name_length = 127
 - max_username_length = 255
 .signup-box.p-3.mb-2
   .signup-body
@@ -7,9 +8,16 @@
         = render "devise/shared/error_messages", resource: resource
       - if Feature.enabled?(:invisible_captcha)
         = invisible_captcha
+      .name.form-row
+        .col.form-group
+          = f.label :first_name, _('First name'), for: 'new_user_first_name', class: 'label-bold'
+          = f.text_field :first_name, class: 'form-control top js-block-emoji js-validate-length', :data => { :max_length => max_first_name_length, :max_length_message => _("First Name is too long (maximum is %{max_length} characters).") % { max_length: max_first_name_length }, :qa_selector => 'new_user_firstname_field' }, required: true, title: _("This field is required.")
+        .col.form-group
+          = f.label :last_name, _('Last name'), for: 'new_user_last_name', class: 'label-bold'
+          = f.text_field :last_name, class: "form-control top js-block-emoji js-validate-length", :data => { :max_length => max_last_name_length, :max_length_message => _("Last Name is too long (maximum is %{max_length} characters).") % { max_length: max_last_name_length }, :qa_selector => 'new_user_lastname_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.")
+        = 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 => _("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.")
         %p.validation-error.gl-field-error-ignore.field-validation.mt-1.hide.cred= _('Username is already taken.')
         %p.validation-success.gl-field-error-ignore.field-validation.mt-1.hide.cgreen= _('Username is available.')
         %p.validation-pending.gl-field-error-ignore.field-validation.mt-1.hide= _('Checking username availability...')

app/views/devise/shared/_signup_box.html.haml

-- max_name_length = 128
+- max_name_length = 255
 - max_username_length = 255
 #register-pane.tab-pane.login-box{ role: 'tabpanel' }
   .login-body

app/views/registrations/welcome.html.haml

-- content_for(:page_title, _('Welcome to GitLab @%{username}!') % { username: current_user.username })
-- max_name_length = 128
+- content_for(:page_title, _('Welcome to GitLab %{name}!') % { name: current_user.name })
 .text-center.mb-3
   = _('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
@@ -7,9 +6,6 @@
     = 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|
       .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'

changelogs/unreleased/nicolasdular-split-signup-full-name.yml

+---
+title: Update name max length
+merge_request: 22840
+author:
+type: changed

ee/app/services/ee/users/build_service.rb

@@ -60,18 +60,6 @@ module EE
         end
       end
 
-      override :build_user_params
-      def build_user_params(skip_authorization:)
-        user_params = super
-        fallback_name = "#{user_params[:first_name]} #{user_params[:last_name]}"
-
-        if user_params[:name].blank? && fallback_name.present?
-          user_params.merge(name: fallback_name)
-        else
-          user_params
-        end
-      end
-
       def saml_provider_id
         strong_memoize(:saml_provider_id) do
           group = GroupFinder.new(current_user).execute(id: group_id_for_saml)

ee/app/views/trial_registrations/_signup_box.html.haml

-- max_name_length = 128
+- max_first_name_length = max_last_name_length = 127
 - max_username_length = 255
 #register-pane.tab-pane.login-box{ role: 'tabpanel' }
   .login-body
@@ -10,13 +10,13 @@
       .name.form-row
         .col.form-group
           = f.label :first_name, _('First name'), for: 'new_user_first_name', class: 'label-bold'
-          = f.text_field :first_name, class: 'form-control top js-block-emoji js-validate-length', :data => { :max_length => max_name_length, :max_length_message => s_("SignUp|First Name is too long (maximum is %{max_length} characters).") % { max_length: max_name_length }, :qa_selector => 'new_user_firstname_field' }, required: true, title: _("This field is required.")
+          = f.text_field :first_name, class: 'form-control top js-block-emoji js-validate-length', :data => { :max_length => max_first_name_length, :max_length_message => s_("SignUp|First Name is too long (maximum is %{max_length} characters).") % { max_length: max_first_name_length }, :qa_selector => 'new_user_firstname_field' }, required: true, title: _("This field is required.")
         .col.form-group
           = f.label :last_name, _('Last name'), for: 'new_user_last_name', class: 'label-bold'
-          = f.text_field :last_name, class: "form-control top js-block-emoji js-validate-length", :data => { :max_length => max_name_length, :max_length_message => s_("SignUp|Last Name is too long (maximum is %{max_length} characters).") % { max_length: max_name_length }, :qa_selector => 'new_user_lastname_field' }, required: true, title: _("This field is required.")
+          = f.text_field :last_name, class: "form-control top js-block-emoji js-validate-length", :data => { :max_length => max_last_name_length, :max_length_message => s_("SignUp|Last Name is too long (maximum is %{max_length} characters).") % { max_length: max_last_name_length }, :qa_selector => 'new_user_lastname_field' }, required: true, title: _("This field is required.")
       .username.form-group
         = f.label :username, for: 'new_user_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, :api_path => suggestion_path, :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.")
+        = f.text_field :username, class: 'form-control middle js-block-emoji js-validate-length js-validate-username', :data => { :max_length => max_username_length, :api_path => suggestion_path, :max_length_message => s_("SignUp|Username is too long (maximum is %{max_length} characters).") % { max_last_name_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.")
         %p.validation-error.gl-field-error-ignore.field-validation.hide= _('Username is already taken.')
         %p.validation-success.gl-field-error-ignore.field-validation.hide= _('Username is available.')
         %p.validation-pending.gl-field-error-ignore.field-validation.hide= _('Checking username availability...')

locale/gitlab.pot

@@ -8153,6 +8153,9 @@ msgstr ""
 msgid "Finished"
 msgstr ""
 
+msgid "First Name is too long (maximum is %{max_length} characters)."
+msgstr ""
+
 msgid "First Seen"
 msgstr ""
 
@@ -10682,6 +10685,9 @@ msgstr ""
 msgid "Last Accessed On"
 msgstr ""
 
+msgid "Last Name is too long (maximum is %{max_length} characters)."
+msgstr ""
+
 msgid "Last Pipeline"
 msgstr ""
 
@@ -12005,9 +12011,6 @@ msgstr ""
 msgid "Name has already been taken"
 msgstr ""
 
-msgid "Name is too long (maximum is %{max_length} characters)."
-msgstr ""
-
 msgid "Name new label"
 msgstr ""
 
@@ -20475,6 +20478,9 @@ msgstr ""
 msgid "Username is available."
 msgstr ""
 
+msgid "Username is too long (maximum is %{max_length} characters)."
+msgstr ""
+
 msgid "Username or email"
 msgstr ""
 
@@ -20849,7 +20855,7 @@ msgstr ""
 msgid "Welcome to GitLab"
 msgstr ""
 
-msgid "Welcome to GitLab @%{username}!"
+msgid "Welcome to GitLab %{name}!"
 msgstr ""
 
 msgid "Welcome to the Guided GitLab Tour"

spec/controllers/registrations_controller_spec.rb

@@ -306,6 +306,23 @@ describe RegistrationsController do
 
       expect(subject.current_user).not_to be_nil
     end
+
+    context 'with the experimental signup flow enabled and the user is part of the experimental group' do
+      before do
+        stub_experiment(signup_flow: true)
+        stub_experiment_for_user(signup_flow: true)
+      end
+
+      let(:base_user_params) { { first_name: 'First', last_name: 'Last', username: 'new_username', email: 'new@user.com', password: 'Any_password' } }
+
+      it 'sets name from first and last name' do
+        post :create, params: { new_user: base_user_params }
+
+        expect(User.last.first_name).to eq(base_user_params[:first_name])
+        expect(User.last.last_name).to eq(base_user_params[:last_name])
+        expect(User.last.name).to eq("#{base_user_params[:first_name]} #{base_user_params[:last_name]}")
+      end
+    end
   end
 
   describe '#destroy' do
@@ -395,7 +412,7 @@ describe RegistrationsController do
         label: anything,
         property: 'experimental_group'
       )
-      patch :update_registration, params: { user: { name: 'New name', role: 'software_developer', setup_for_company: 'false' } }
+      patch :update_registration, params: { user: { role: 'software_developer', setup_for_company: 'false' } }
     end
   end
 end

spec/features/users/signup_spec.rb

@@ -123,50 +123,6 @@ shared_examples 'Signup' do
     end
   end
 
-  describe 'user\'s full name validation', :js do
-    before do
-      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 @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("##{@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 @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 @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("##{@user_name_field}", 'Ehsan 🦋')
-
-      expect(page).to have_content("Invalid input, please avoid emojis")
-    end
-  end
-
   context 'with no errors' do
     context 'when sending confirmation email' do
       before do
@@ -184,7 +140,10 @@ shared_examples 'Signup' do
           fill_in 'new_user_username', with: new_user.username
           fill_in 'new_user_email', with: new_user.email
 
-          unless Gitlab::Experimentation.enabled?(:signup_flow)
+          if Gitlab::Experimentation.enabled?(:signup_flow)
+            fill_in 'new_user_first_name', with: new_user.first_name
+            fill_in 'new_user_last_name', with: new_user.last_name
+          else
             fill_in 'new_user_name', with: new_user.name
             fill_in 'new_user_email_confirmation', with: new_user.email
           end
@@ -209,7 +168,10 @@ shared_examples 'Signup' do
           fill_in 'new_user_username', with: new_user.username
           fill_in 'new_user_email', with: new_user.email
 
-          unless Gitlab::Experimentation.enabled?(:signup_flow)
+          if Gitlab::Experimentation.enabled?(:signup_flow)
+            fill_in 'new_user_first_name', with: new_user.first_name
+            fill_in 'new_user_last_name', with: new_user.last_name
+          else
             fill_in 'new_user_name', with: new_user.name
             fill_in 'new_user_email_confirmation', with: new_user.email
           end
@@ -235,7 +197,10 @@ shared_examples 'Signup' do
         fill_in 'new_user_username', with: new_user.username
         fill_in 'new_user_email', with: new_user.email
 
-        unless Gitlab::Experimentation.enabled?(:signup_flow)
+        if Gitlab::Experimentation.enabled?(:signup_flow)
+          fill_in 'new_user_first_name', with: new_user.first_name
+          fill_in 'new_user_last_name', with: new_user.last_name
+        else
           fill_in 'new_user_name', with: new_user.name
           fill_in 'new_user_email_confirmation', with: new_user.email.capitalize
         end
@@ -263,7 +228,10 @@ shared_examples 'Signup' do
         fill_in 'new_user_username', with: new_user.username
         fill_in 'new_user_email', with: new_user.email
 
-        unless Gitlab::Experimentation.enabled?(:signup_flow)
+        if Gitlab::Experimentation.enabled?(:signup_flow)
+          fill_in 'new_user_first_name', with: new_user.first_name
+          fill_in 'new_user_last_name', with: new_user.last_name
+        else
           fill_in 'new_user_name', with: new_user.name
           fill_in 'new_user_email_confirmation', with: new_user.email
         end
@@ -287,7 +255,10 @@ shared_examples 'Signup' do
 
       visit new_user_registration_path
 
-      unless Gitlab::Experimentation.enabled?(:signup_flow)
+      if Gitlab::Experimentation.enabled?(:signup_flow)
+        fill_in 'new_user_first_name', with: new_user.first_name
+        fill_in 'new_user_last_name', with: new_user.last_name
+      else
         fill_in 'new_user_name', with: new_user.name
       end
 
@@ -313,7 +284,10 @@ shared_examples 'Signup' do
 
       visit new_user_registration_path
 
-      unless Gitlab::Experimentation.enabled?(:signup_flow)
+      if Gitlab::Experimentation.enabled?(:signup_flow)
+        fill_in 'new_user_first_name', with: new_user.first_name
+        fill_in 'new_user_last_name', with: new_user.last_name
+      else
         fill_in 'new_user_name', with: new_user.name
       end
 
@@ -338,7 +312,10 @@ shared_examples 'Signup' do
       fill_in 'new_user_username', with: new_user.username
       fill_in 'new_user_email', with: new_user.email
 
-      unless Gitlab::Experimentation.enabled?(:signup_flow)
+      if Gitlab::Experimentation.enabled?(:signup_flow)
+        fill_in 'new_user_first_name', with: new_user.first_name
+        fill_in 'new_user_last_name', with: new_user.last_name
+      else
         fill_in 'new_user_name', with: new_user.name
         fill_in 'new_user_email_confirmation', with: new_user.email
       end
@@ -357,7 +334,10 @@ shared_examples 'Signup' do
       fill_in 'new_user_username', with: new_user.username
       fill_in 'new_user_email', with: new_user.email
 
-      unless Gitlab::Experimentation.enabled?(:signup_flow)
+      if Gitlab::Experimentation.enabled?(:signup_flow)
+        fill_in 'new_user_first_name', with: new_user.first_name
+        fill_in 'new_user_last_name', with: new_user.last_name
+      else
         fill_in 'new_user_name', with: new_user.name
         fill_in 'new_user_email_confirmation', with: new_user.email
       end
@@ -394,7 +374,10 @@ shared_examples 'Signup' do
       fill_in 'new_user_username', with: new_user.username
       fill_in 'new_user_email', with: new_user.email
 
-      unless Gitlab::Experimentation.enabled?(:signup_flow)
+      if Gitlab::Experimentation.enabled?(:signup_flow)
+        fill_in 'new_user_first_name', with: new_user.first_name
+        fill_in 'new_user_last_name', with: new_user.last_name
+      else
         fill_in 'new_user_name', with: new_user.name
         fill_in 'new_user_email_confirmation', with: new_user.email
       end
@@ -412,6 +395,44 @@ shared_examples 'Signup' do
   end
 end
 
+shared_examples 'Signup name validation' do |field, max_length|
+  before do
+    visit new_user_registration_path
+  end
+
+  describe "#{field} validation", :js do
+    it "does not show an error border if the user's fullname length is not longer than #{max_length} characters" do
+      fill_in field, with: 'u' * max_length
+
+      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("##{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 #{max_length} characters" do
+      fill_in field, with: 'n' * (max_length + 1)
+
+      expect(find('.name')).to have_css '.gl-field-error-outline'
+    end
+
+    it "shows an error message if the user\'s fullname is longer than #{max_length} characters" do
+      fill_in field, with: 'n' * (max_length + 1)
+
+      expect(page).to have_content("Name is too long (maximum is #{max_length} characters).")
+    end
+
+    it 'shows an error message if the username contains emojis' do
+      simulate_input("##{field}", 'Ehsan 🦋')
+
+      expect(page).to have_content("Invalid input, please avoid emojis")
+    end
+  end
+end
+
 describe 'With original flow' do
   before do
     stub_experiment(signup_flow: false)
@@ -419,6 +440,7 @@ describe 'With original flow' do
   end
 
   it_behaves_like 'Signup'
+  it_behaves_like 'Signup name validation', 'new_user_name', 255
 end
 
 describe 'With experimental flow' do
@@ -428,11 +450,15 @@ describe 'With experimental flow' do
   end
 
   it_behaves_like 'Signup'
+  it_behaves_like 'Signup name validation', 'new_user_first_name', 127
+  it_behaves_like 'Signup name validation', 'new_user_last_name', 127
 
   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_first_name', with: new_user.first_name
+      fill_in 'new_user_last_name', with: new_user.last_name
       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
@@ -441,13 +467,11 @@ describe 'With experimental flow' do
 
       expect(page).to have_current_path(users_sign_up_welcome_path)
 
-      fill_in 'user_name', with: 'New name'
       select 'Software Developer', from: 'user_role'
       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)

spec/models/user_spec.rb

@@ -147,15 +147,15 @@ describe User, :do_not_mock_admin_mode do
 
     describe 'name' do
       it { is_expected.to validate_presence_of(:name) }
-      it { is_expected.to validate_length_of(:name).is_at_most(128) }
+      it { is_expected.to validate_length_of(:name).is_at_most(255) }
     end
 
     describe 'first name' do
-      it { is_expected.to validate_length_of(:first_name).is_at_most(255) }
+      it { is_expected.to validate_length_of(:first_name).is_at_most(127) }
     end
 
     describe 'last name' do
-      it { is_expected.to validate_length_of(:last_name).is_at_most(255) }
+      it { is_expected.to validate_length_of(:last_name).is_at_most(127) }
     end
 
     describe 'username' do