From 1c0521b395acaa0b6f7c58da472158dce239edaa 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 1/7] 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 40dca556c16f1e449c1be6d81014b4f044adc889 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 2/7] 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 a8c1cadf76ccdf58b3858491bb73bdd41c885b5d 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 3/7] 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 36f3a7f3ad73a9..3d9ec62fd11b4b 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -17132,6 +17132,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 afce335fcb1eb551d4f999575fdc81a12b2b0ac1 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 4/7] 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 23a709f58d3bb5095774a0f4eeaa9b86c4afc139 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 5/7] 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 3d9ec62fd11b4b..463bd5bf191cfa 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -17132,9 +17132,6 @@ msgstr "" msgid "First name" msgstr "" -msgid "First name or last name can't be empty" -msgstr "" - msgid "First seen" msgstr "" @@ -37968,6 +37965,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 8a57669f36cf21c1427a7fc8d09ed6925be0d79a 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 6/7] 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 80d7280a1df318af3838a02d3c8ca25d1ab9bf63 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 7/7] 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 463bd5bf191cfa..039121dd04e0f1 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -37965,9 +37965,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 "" @@ -47613,6 +47610,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