From a0410a81c33f1d30d7a317b97a4883a3c5e6db61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Wed, 2 Nov 2022 11:37:38 +0800 Subject: [PATCH 01/10] Reject first/last name that contain only spaces --- app/views/devise/shared/_signup_box.html.haml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/views/devise/shared/_signup_box.html.haml b/app/views/devise/shared/_signup_box.html.haml index b9c9c99bf1a238..491954269e2c13 100644 --- a/app/views/devise/shared/_signup_box.html.haml +++ b/app/views/devise/shared/_signup_box.html.haml @@ -1,6 +1,7 @@ - max_first_name_length = max_last_name_length = 127 - omniauth_providers_placement ||= :bottom - borderless ||= false +- regex_pattern_has_non_space_character = '.*\S+.*' .gl-mb-3.gl-p-4{ class: (borderless ? '' : 'gl-border-gray-100 gl-border-1 gl-border-solid gl-rounded-base') } - if show_omniauth_providers && omniauth_providers_placement == :top @@ -19,6 +20,7 @@ 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_first_name_field' }, + pattern: regex_pattern_has_non_space_character, required: true, title: _('This field is required.') .col.form-group @@ -28,6 +30,7 @@ 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_last_name_field' }, + pattern: regex_pattern_has_non_space_character, required: true, title: _('This field is required.') .username.form-group -- GitLab From 7d079aa473ec1c68bd3e797f3e8258217033d1bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Mon, 7 Nov 2022 22:09:01 +0800 Subject: [PATCH 02/10] Validate name on backend instead of frontend --- app/controllers/registrations_controller.rb | 8 ++++++++ app/views/devise/shared/_signup_box.html.haml | 3 --- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 995303a631afb6..200dbf314a5e98 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -15,6 +15,7 @@ class RegistrationsController < Devise::RegistrationsController layout 'devise' prepend_before_action :check_captcha, only: :create + before_action :ensure_first_name_and_last_name_no_empty, only: :create before_action :ensure_destroy_prerequisites_met, only: [:destroy] before_action :init_preferred_language, only: :new before_action :load_recaptcha, only: :new @@ -172,6 +173,13 @@ def check_captcha render action: 'new' end + def ensure_first_name_and_last_name_no_empty + return if params[resource_name][:first_name].present? && params[resource_name][:last_name].present? + + flash[:alert] = _("First name or last name can't be empty") + render action: 'new' + end + def pending_approval? return false unless Gitlab::CurrentSettings.require_admin_approval_after_user_signup diff --git a/app/views/devise/shared/_signup_box.html.haml b/app/views/devise/shared/_signup_box.html.haml index 491954269e2c13..b9c9c99bf1a238 100644 --- a/app/views/devise/shared/_signup_box.html.haml +++ b/app/views/devise/shared/_signup_box.html.haml @@ -1,7 +1,6 @@ - max_first_name_length = max_last_name_length = 127 - omniauth_providers_placement ||= :bottom - borderless ||= false -- regex_pattern_has_non_space_character = '.*\S+.*' .gl-mb-3.gl-p-4{ class: (borderless ? '' : 'gl-border-gray-100 gl-border-1 gl-border-solid gl-rounded-base') } - if show_omniauth_providers && omniauth_providers_placement == :top @@ -20,7 +19,6 @@ 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_first_name_field' }, - pattern: regex_pattern_has_non_space_character, required: true, title: _('This field is required.') .col.form-group @@ -30,7 +28,6 @@ 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_last_name_field' }, - pattern: regex_pattern_has_non_space_character, required: true, title: _('This field is required.') .username.form-group -- GitLab From b9561e879c2539ed78c55a10c3929502aae207b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Mon, 7 Nov 2022 22:44:11 +0800 Subject: [PATCH 03/10] Add test for register about first/last name --- locale/gitlab.pot | 3 ++ .../registrations_controller_spec.rb | 31 +++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 78b870a8266bfc..758cedd06449e3 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -17202,6 +17202,9 @@ msgstr "" msgid "First name" msgstr "" +msgid "First name or last name can't be empty" +msgstr "" + msgid "First seen" msgstr "" diff --git a/spec/controllers/registrations_controller_spec.rb b/spec/controllers/registrations_controller_spec.rb index 8775f68a5dea82..67f18923e934bb 100644 --- a/spec/controllers/registrations_controller_spec.rb +++ b/spec/controllers/registrations_controller_spec.rb @@ -552,6 +552,37 @@ end end end + + context 'when the first or last name is not "present?"' do + using RSpec::Parameterized::TableSyntax + + shared_examples 'a user without present first name or last name' do + it 'renders the form with errors' do + subject + expect(controller.current_user).to be_nil + expect(response).to render_template(:new) + expect(flash[:alert]).to eq(_("First name or last name can't be empty")) + end + end + + where(:first_name, :last_name) do + nil | 'last' + '' | 'last' + ' ' | 'last' + 'first' | nil + 'first' | '' + 'first' | ' ' + '' | '' + end + + with_them do + before do + base_user_params.merge!({ first_name: first_name, last_name: last_name }) + end + + it_behaves_like 'a user without present first name or last name' + end + end end describe '#destroy' do -- GitLab From 3d38c40b79eec6cdd6431ad73cc1564345234807 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Tue, 8 Nov 2022 23:09:06 +0800 Subject: [PATCH 04/10] Rename function: 'xxx_no_empty' to 'xxx_not_empty' --- app/controllers/registrations_controller.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 200dbf314a5e98..f82649022ab25d 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -15,7 +15,7 @@ class RegistrationsController < Devise::RegistrationsController layout 'devise' prepend_before_action :check_captcha, only: :create - before_action :ensure_first_name_and_last_name_no_empty, only: :create + before_action :ensure_first_name_and_last_name_not_empty, only: :create before_action :ensure_destroy_prerequisites_met, only: [:destroy] before_action :init_preferred_language, only: :new before_action :load_recaptcha, only: :new @@ -173,7 +173,7 @@ def check_captcha render action: 'new' end - def ensure_first_name_and_last_name_no_empty + def ensure_first_name_and_last_name_not_empty return if params[resource_name][:first_name].present? && params[resource_name][:last_name].present? flash[:alert] = _("First name or last name can't be empty") -- GitLab From 8ee7fd1554dcca256e2297113525a9d7d480e144 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Fri, 11 Nov 2022 14:57:15 +0800 Subject: [PATCH 05/10] Add 'SignUp|' prefix for i18n text --- app/controllers/registrations_controller.rb | 2 +- locale/gitlab.pot | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index f82649022ab25d..e082ddb0c93a64 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -176,7 +176,7 @@ def check_captcha def ensure_first_name_and_last_name_not_empty return if params[resource_name][:first_name].present? && params[resource_name][:last_name].present? - flash[:alert] = _("First name or last name can't be empty") + flash[:alert] = s_("SignUp|First name or last name can't be empty") render action: 'new' end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 758cedd06449e3..af85af5aaf3186 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -17202,9 +17202,6 @@ msgstr "" msgid "First name" msgstr "" -msgid "First name or last name can't be empty" -msgstr "" - msgid "First seen" msgstr "" @@ -38220,6 +38217,9 @@ msgstr "" msgid "SignUp|First name is too long (maximum is %{max_length} characters)." msgstr "" +msgid "SignUp|First name or last name can't be empty" +msgstr "" + msgid "SignUp|Last name is too long (maximum is %{max_length} characters)." msgstr "" -- GitLab From 5da74b625f644792d0ca7568ef2454fc7b0a9c59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Mon, 14 Nov 2022 10:49:23 +0800 Subject: [PATCH 06/10] Fix unittest of register - error msg --- spec/features/invites_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/invites_spec.rb b/spec/features/invites_spec.rb index 34990a53b517d0..58920f73d3a64b 100644 --- a/spec/features/invites_spec.rb +++ b/spec/features/invites_spec.rb @@ -211,7 +211,7 @@ def fill_in_welcome_form it 'fails sign up and redirects back to sign up', :aggregate_failures do expect { fill_in_sign_up_form(new_user) }.not_to change { User.count } - expect(page).to have_content('prohibited this user from being saved') + expect(page).to have_content("First name or last name can't be empty") expect(page).to have_current_path(user_registration_path, ignore_query: true) end end -- GitLab From 44bed410671e7e7c02e0d1f5a9c2e1db2f23904a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Wed, 16 Nov 2022 16:45:19 +0800 Subject: [PATCH 07/10] Put errors in errors instead of flash --- app/controllers/registrations_controller.rb | 3 ++- locale/gitlab.pot | 6 +++--- spec/controllers/registrations_controller_spec.rb | 4 +++- spec/features/invites_spec.rb | 2 +- 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index e082ddb0c93a64..35f395ac904ac0 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -176,7 +176,8 @@ def check_captcha def ensure_first_name_and_last_name_not_empty return if params[resource_name][:first_name].present? && params[resource_name][:last_name].present? - flash[:alert] = s_("SignUp|First name or last name can't be empty") + resource.errors.add(_('First name'), _("cannot be blank")) if params[resource_name][:first_name].blank? + resource.errors.add(_('Last name'), _("cannot be blank")) if params[resource_name][:last_name].blank? render action: 'new' end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index af85af5aaf3186..de0aacb2c6699e 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -38217,9 +38217,6 @@ msgstr "" msgid "SignUp|First name is too long (maximum is %{max_length} characters)." msgstr "" -msgid "SignUp|First name or last name can't be empty" -msgstr "" - msgid "SignUp|Last name is too long (maximum is %{max_length} characters)." msgstr "" @@ -47941,6 +47938,9 @@ msgstr "" msgid "cannot be associated with both a Group and a Project" msgstr "" +msgid "cannot be blank" +msgstr "" + msgid "cannot be changed" msgstr "" diff --git a/spec/controllers/registrations_controller_spec.rb b/spec/controllers/registrations_controller_spec.rb index 67f18923e934bb..489461d8876cee 100644 --- a/spec/controllers/registrations_controller_spec.rb +++ b/spec/controllers/registrations_controller_spec.rb @@ -556,12 +556,14 @@ context 'when the first or last name is not "present?"' do using RSpec::Parameterized::TableSyntax + render_views + shared_examples 'a user without present first name or last name' do it 'renders the form with errors' do subject expect(controller.current_user).to be_nil expect(response).to render_template(:new) - expect(flash[:alert]).to eq(_("First name or last name can't be empty")) + expect(response.body).to include(_('name cannot be blank')) # include 'First name' or 'Last name' or both end end diff --git a/spec/features/invites_spec.rb b/spec/features/invites_spec.rb index 58920f73d3a64b..34990a53b517d0 100644 --- a/spec/features/invites_spec.rb +++ b/spec/features/invites_spec.rb @@ -211,7 +211,7 @@ def fill_in_welcome_form it 'fails sign up and redirects back to sign up', :aggregate_failures do expect { fill_in_sign_up_form(new_user) }.not_to change { User.count } - expect(page).to have_content("First name or last name can't be empty") + expect(page).to have_content('prohibited this user from being saved') expect(page).to have_current_path(user_registration_path, ignore_query: true) end end -- GitLab From dc709ad37bd8bba817878d796cb1d28d14dbb63e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Mon, 28 Nov 2022 16:55:13 +0800 Subject: [PATCH 08/10] Add blank line before render --- app/controllers/registrations_controller.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 35f395ac904ac0..753bed2953ad44 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -178,6 +178,7 @@ def ensure_first_name_and_last_name_not_empty resource.errors.add(_('First name'), _("cannot be blank")) if params[resource_name][:first_name].blank? resource.errors.add(_('Last name'), _("cannot be blank")) if params[resource_name][:last_name].blank? + render action: 'new' end -- GitLab From 7ca0818f94b561764cc793af119e0144bafb311b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Tue, 29 Nov 2022 13:36:17 +0800 Subject: [PATCH 09/10] Use 'dig' to prevent errors from nil --- app/controllers/registrations_controller.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 753bed2953ad44..adeb3e3b26f569 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -174,10 +174,13 @@ def check_captcha end def ensure_first_name_and_last_name_not_empty - return if params[resource_name][:first_name].present? && params[resource_name][:last_name].present? + first_name = params.dig(resource_name, :first_name) + last_name = params.dig(resource_name, :last_name) - resource.errors.add(_('First name'), _("cannot be blank")) if params[resource_name][:first_name].blank? - resource.errors.add(_('Last name'), _("cannot be blank")) if params[resource_name][:last_name].blank? + return if first_name.present? && last_name.present? + + resource.errors.add(_('First name'), _("cannot be blank")) if first_name.blank? + resource.errors.add(_('Last name'), _("cannot be blank")) if last_name.blank? render action: 'new' end -- GitLab From 37553b73f7530ac95a8ce3b8d90ede2b97de5929 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B7=AF=E5=B0=8F=E9=B9=BF?= <1551755561@qq.com> Date: Tue, 6 Dec 2022 21:42:26 +0800 Subject: [PATCH 10/10] Fix: handle the 'new_' prefix of key --- app/controllers/registrations_controller.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index adeb3e3b26f569..2be7b72159e9ba 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -174,8 +174,11 @@ def check_captcha end def ensure_first_name_and_last_name_not_empty - first_name = params.dig(resource_name, :first_name) - last_name = params.dig(resource_name, :last_name) + # The key here will be affected by feature flag 'arkose_labs_signup_challenge' + # When flag is disabled, the key will be 'user' because #check_captcha will remove 'new_' prefix + # When flag is enabled, #check_captcha will be skipped, so the key will have 'new_' prefix + first_name = params.dig(resource_name, :first_name) || params.dig("new_#{resource_name}", :first_name) + last_name = params.dig(resource_name, :last_name) || params.dig("new_#{resource_name}", :last_name) return if first_name.present? && last_name.present? -- GitLab