From e72419870257df9bb464190f8478d261c2b28ad3 Mon Sep 17 00:00:00 2001
From: Nicolas Dular <ndular@gitlab.com>
Date: Thu, 17 Sep 2020 12:25:14 +0200
Subject: [PATCH] Remove accept terms checkbox for signup
To remove obstacles in our signup process, we now only render a text
that states that the user accepts the terms of service when clicking
register instead of checking the checkbox.
---
app/controllers/registrations_controller.rb | 30 +---
...mental_separate_sign_up_flow_box.html.haml | 11 +-
app/views/devise/shared/_signup_box.html.haml | 8 +-
.../shared/_terms_of_service_notice.html.haml | 5 +
.../nicolasdular-remove-tos-checkobx.yml | 5 +
.../trial_registrations/_signup_box.html.haml | 6 +-
.../trial_registrations/signup_spec.rb | 2 -
lib/gitlab/experimentation.rb | 3 -
locale/gitlab.pot | 8 +-
qa/qa/page/main/sign_up.rb | 3 -
.../registrations_controller_spec.rb | 136 ++----------------
spec/features/users/signup_spec.rb | 39 +----
12 files changed, 30 insertions(+), 226 deletions(-)
create mode 100644 app/views/devise/shared/_terms_of_service_notice.html.haml
create mode 100644 changelogs/unreleased/nicolasdular-remove-tos-checkobx.yml
diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb
index 139ef5e0692d8..037c83b062b98 100644
--- a/app/controllers/registrations_controller.rb
+++ b/app/controllers/registrations_controller.rb
@@ -13,16 +13,12 @@ class RegistrationsController < Devise::RegistrationsController
skip_before_action :required_signup_info, :check_two_factor_requirement, only: [:welcome, :update_registration]
prepend_before_action :check_captcha, only: :create
before_action :whitelist_query_limiting, :ensure_destroy_prerequisites_met, only: [:destroy]
- before_action :ensure_terms_accepted,
- if: -> { action_name == 'create' && Gitlab::CurrentSettings.current_application_settings.enforce_terms? }
before_action :load_recaptcha, only: :new
feature_category :authentication_and_authorization
def new
if experiment_enabled?(:signup_flow)
- track_experiment_event(:terms_opt_in, 'start')
-
@resource = build_resource
else
redirect_to new_user_session_path(anchor: 'register-pane')
@@ -36,7 +32,6 @@ def create
super do |new_user|
persist_accepted_terms_if_required(new_user)
set_role_required(new_user)
- track_terms_experiment(new_user)
yield new_user if block_given?
end
@@ -86,10 +81,8 @@ def persist_accepted_terms_if_required(new_user)
return unless new_user.persisted?
return unless Gitlab::CurrentSettings.current_application_settings.enforce_terms?
- if terms_accepted?
- terms = ApplicationSetting::Term.latest
- Users::RespondToTermsService.new(new_user, terms).execute(accepted: true)
- end
+ terms = ApplicationSetting::Term.latest
+ Users::RespondToTermsService.new(new_user, terms).execute(accepted: true)
end
def set_role_required(new_user)
@@ -185,18 +178,6 @@ def whitelist_query_limiting
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42380')
end
- def ensure_terms_accepted
- return if terms_accepted?
-
- redirect_to new_user_session_path, alert: _('You must accept our Terms of Service and privacy policy in order to register an account')
- end
-
- def terms_accepted?
- return true if experiment_enabled?(:terms_opt_in)
-
- Gitlab::Utils.to_boolean(params[:terms_opt_in])
- end
-
def path_for_signed_in_user(user)
if requires_confirmation?(user)
users_almost_there_path
@@ -213,13 +194,6 @@ def requires_confirmation?(user)
true
end
- def track_terms_experiment(new_user)
- return unless new_user.persisted?
-
- track_experiment_event(:terms_opt_in, 'end')
- record_experiment_user(:terms_opt_in)
- end
-
def load_recaptcha
Gitlab::Recaptcha.load_configurations!
end
diff --git a/app/views/devise/shared/_experimental_separate_sign_up_flow_box.html.haml b/app/views/devise/shared/_experimental_separate_sign_up_flow_box.html.haml
index ecabb4495cce0..621bbb32a1394 100644
--- a/app/views/devise/shared/_experimental_separate_sign_up_flow_box.html.haml
+++ b/app/views/devise/shared/_experimental_separate_sign_up_flow_box.html.haml
@@ -28,21 +28,12 @@
= f.label :password, class: 'label-bold'
= f.password_field :password, class: "form-control bottom", data: { qa_selector: 'new_user_password_field' }, required: true, pattern: ".{#{@minimum_password_length},}", title: _("Minimum length is %{minimum_password_length} characters.") % { minimum_password_length: @minimum_password_length }
%p.gl-field-hint.text-secondary= _('Minimum length is %{minimum_password_length} characters') % { minimum_password_length: @minimum_password_length }
- - if Gitlab::CurrentSettings.current_application_settings.enforce_terms? && !experiment_enabled?(:terms_opt_in)
- .form-group
- = check_box_tag :terms_opt_in, '1', false, required: true, data: { qa_selector: 'new_user_accept_terms_checkbox' }
- = label_tag :terms_opt_in do
- - terms_link = link_to s_("I accept the|Terms of Service and Privacy Policy"), terms_path, target: "_blank"
- - accept_terms_label = _("I accept the %{terms_link}") % { terms_link: terms_link }
- = accept_terms_label.html_safe
= render_if_exists 'devise/shared/email_opted_in', f: f
%div
- if show_recaptcha_sign_up?
= recaptcha_tags
.submit-container.mt-3
= f.submit _("Register"), class: "btn-register btn btn-block btn-success mb-0 p-2", data: { qa_selector: 'new_user_register_button' }
- - if experiment_enabled?(:terms_opt_in)
- %p.gl-text-gray-500.gl-mt-5.gl-mb-0
- = html_escape(_("By clicking Register, I agree that I have read and accepted the GitLab %{linkStart}Terms of Use and Privacy Policy%{linkEnd}")) % { linkStart: "<a href='#{terms_path}' target='_blank' rel='noreferrer noopener'>".html_safe, linkEnd: '</a>'.html_safe }
+ = render 'devise/shared/terms_of_service_notice'
- if omniauth_enabled? && button_based_providers_enabled?
= render 'devise/shared/experimental_separate_sign_up_flow_omniauth_box'
diff --git a/app/views/devise/shared/_signup_box.html.haml b/app/views/devise/shared/_signup_box.html.haml
index 1fd81cdbac262..f4ac9ad696bd5 100644
--- a/app/views/devise/shared/_signup_box.html.haml
+++ b/app/views/devise/shared/_signup_box.html.haml
@@ -28,16 +28,10 @@
= f.label :password, class: 'label-bold'
= f.password_field :password, class: "form-control bottom", data: { qa_selector: 'new_user_password_field' }, required: true, pattern: ".{#{@minimum_password_length},}", title: _("Minimum length is %{minimum_password_length} characters.") % { minimum_password_length: @minimum_password_length }
%p.gl-field-hint.text-secondary= _('Minimum length is %{minimum_password_length} characters') % { minimum_password_length: @minimum_password_length }
- - if Gitlab::CurrentSettings.current_application_settings.enforce_terms?
- .form-group
- = check_box_tag :terms_opt_in, '1', false, required: true, data: { qa_selector: 'new_user_accept_terms_checkbox' }
- = label_tag :terms_opt_in do
- - terms_link = link_to s_("I accept the|Terms of Service and Privacy Policy"), terms_path, target: "_blank"
- - accept_terms_label = _("I accept the %{terms_link}") % { terms_link: terms_link }
- = accept_terms_label.html_safe
= render_if_exists 'devise/shared/email_opted_in', f: f
%div
- if show_recaptcha_sign_up?
= recaptcha_tags
.submit-container
= f.submit _("Register"), class: "btn-register btn", data: { qa_selector: 'new_user_register_button' }
+ = render 'devise/shared/terms_of_service_notice'
diff --git a/app/views/devise/shared/_terms_of_service_notice.html.haml b/app/views/devise/shared/_terms_of_service_notice.html.haml
new file mode 100644
index 0000000000000..46b043b283180
--- /dev/null
+++ b/app/views/devise/shared/_terms_of_service_notice.html.haml
@@ -0,0 +1,5 @@
+- company_name = Gitlab.com? ? 'GitLab' : ''
+
+- if Gitlab::CurrentSettings.current_application_settings.enforce_terms?
+ %p.gl-text-gray-500.gl-mt-5.gl-mb-0
+ = html_escape(_("By clicking Register, I agree that I have read and accepted the %{company_name} %{linkStart}Terms of Use and Privacy Policy%{linkEnd}")) % { linkStart: "<a href='#{terms_path}' target='_blank' rel='noreferrer noopener'>".html_safe, linkEnd: '</a>'.html_safe, company_name: company_name }
diff --git a/changelogs/unreleased/nicolasdular-remove-tos-checkobx.yml b/changelogs/unreleased/nicolasdular-remove-tos-checkobx.yml
new file mode 100644
index 0000000000000..704bc965ff4bf
--- /dev/null
+++ b/changelogs/unreleased/nicolasdular-remove-tos-checkobx.yml
@@ -0,0 +1,5 @@
+---
+title: Remove accept terms checkbox for signup
+merge_request: 42581
+author:
+type: changed
diff --git a/ee/app/views/trial_registrations/_signup_box.html.haml b/ee/app/views/trial_registrations/_signup_box.html.haml
index dc51f3860158c..ec880154ba5d0 100644
--- a/ee/app/views/trial_registrations/_signup_box.html.haml
+++ b/ee/app/views/trial_registrations/_signup_box.html.haml
@@ -27,11 +27,6 @@
= f.label :password, for: 'new_user_password', class: 'label-bold'
= f.password_field :password, class: 'form-control bottom', data: { qa_selector: 'new_user_password_field' }, required: true, pattern: ".{#{@minimum_password_length},}", title: _("Minimum length is %{minimum_password_length} characters.") % { minimum_password_length: @minimum_password_length }
%p.gl-field-hint.text-secondary= _('Minimum length is %{minimum_password_length} characters') % { minimum_password_length: @minimum_password_length }
- .form-group
- = check_box_tag :terms_opt_in, '1', false, required: true, data: { qa_selector: 'new_user_accept_terms_checkbox' }
- = label_tag :terms_opt_in, for: 'terms_opt_in', class: 'form-check-label' do
- - terms_link_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: terms_path }
- = _("I accept the %{terms_link_start}Terms of Service and Privacy Policy%{terms_link_end}").html_safe % { terms_link_start: terms_link_start, terms_link_end: '</a>'.html_safe }
.form-group
= f.check_box :email_opted_in, data: { qa_selector: 'new_user_email_opted_in_checkbox' }
= f.label :email_opted_in, _("I'd like to receive updates via email about GitLab"), class: 'form-check-label'
@@ -40,3 +35,4 @@
= recaptcha_tags
.submit-container
= f.submit _("Continue"), class: "btn-register btn", data: { qa_selector: 'new_user_register_button' }
+ = render 'devise/shared/terms_of_service_notice'
diff --git a/ee/spec/features/trial_registrations/signup_spec.rb b/ee/spec/features/trial_registrations/signup_spec.rb
index 21c3c1eea0b70..7bb68a2306135 100644
--- a/ee/spec/features/trial_registrations/signup_spec.rb
+++ b/ee/spec/features/trial_registrations/signup_spec.rb
@@ -37,8 +37,6 @@
fill_in 'new_user_email', with: user_attrs[:email]
fill_in 'new_user_password', with: user_attrs[:password]
- check 'terms_opt_in'
-
click_button 'Continue'
end
diff --git a/lib/gitlab/experimentation.rb b/lib/gitlab/experimentation.rb
index 4457c65c519fa..229caebc268d3 100644
--- a/lib/gitlab/experimentation.rb
+++ b/lib/gitlab/experimentation.rb
@@ -51,9 +51,6 @@ module Experimentation
new_create_project_ui: {
tracking_category: 'Manage::Import::Experiment::NewCreateProjectUi'
},
- terms_opt_in: {
- tracking_category: 'Growth::Acquisition::Experiment::TermsOptIn'
- },
contact_sales_btn_in_app: {
tracking_category: 'Growth::Conversion::Experiment::ContactSalesInApp'
},
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 8d38319762e2e..663ed9d4a3cd5 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -4433,7 +4433,7 @@ msgstr ""
msgid "By URL"
msgstr ""
-msgid "By clicking Register, I agree that I have read and accepted the GitLab %{linkStart}Terms of Use and Privacy Policy%{linkEnd}"
+msgid "By clicking Register, I agree that I have read and accepted the %{company_name} %{linkStart}Terms of Use and Privacy Policy%{linkEnd}"
msgstr ""
msgid "By default GitLab sends emails in HTML and plain text formats so mail clients can choose what format to use. Disable this option if you only want to send emails in plain text format."
@@ -13285,9 +13285,6 @@ msgstr ""
msgid "However, you are already a member of this %{member_source}. Sign in using a different account to accept the invitation."
msgstr ""
-msgid "I accept the %{terms_link_start}Terms of Service and Privacy Policy%{terms_link_end}"
-msgstr ""
-
msgid "I accept the %{terms_link}"
msgstr ""
@@ -29781,9 +29778,6 @@ msgstr ""
msgid "You may close the milestone now."
msgstr ""
-msgid "You must accept our Terms of Service and privacy policy in order to register an account"
-msgstr ""
-
msgid "You must be logged in to search across all of GitLab"
msgstr ""
diff --git a/qa/qa/page/main/sign_up.rb b/qa/qa/page/main/sign_up.rb
index 8be3b2291b608..98bbbc530278a 100644
--- a/qa/qa/page/main/sign_up.rb
+++ b/qa/qa/page/main/sign_up.rb
@@ -11,7 +11,6 @@ class SignUp < Page::Base
element :new_user_email_field
element :new_user_password_field
element :new_user_register_button
- element :new_user_accept_terms_checkbox
end
view 'app/views/registrations/welcome.html.haml' do
@@ -25,8 +24,6 @@ def sign_up!(user)
fill_element :new_user_email_field, user.email
fill_element :new_user_password_field, user.password
- check_element :new_user_accept_terms_checkbox if has_element?(:new_user_accept_terms_checkbox)
-
signed_in = retry_until do
click_element :new_user_register_button if has_element?(:new_user_register_button)
diff --git a/spec/controllers/registrations_controller_spec.rb b/spec/controllers/registrations_controller_spec.rb
index c3080b50f443d..cca453fd0754d 100644
--- a/spec/controllers/registrations_controller_spec.rb
+++ b/spec/controllers/registrations_controller_spec.rb
@@ -37,46 +37,6 @@
expect(response).to redirect_to(new_user_session_path(anchor: 'register-pane'))
end
end
-
- context 'with sign up flow and terms_opt_in experiment being enabled' do
- before do
- stub_experiment(signup_flow: true, terms_opt_in: true)
- end
-
- context 'when user is not part of the experiment' do
- before do
- stub_experiment_for_user(signup_flow: true, terms_opt_in: false)
- end
-
- it 'tracks event with right parameters' do
- expect(Gitlab::Tracking).to receive(:event).with(
- 'Growth::Acquisition::Experiment::TermsOptIn',
- 'start',
- label: anything,
- property: 'control_group'
- )
-
- subject
- end
- end
-
- context 'when user is part of the experiment' do
- before do
- stub_experiment_for_user(signup_flow: true, terms_opt_in: true)
- end
-
- it 'tracks event with right parameters' do
- expect(Gitlab::Tracking).to receive(:event).with(
- 'Growth::Acquisition::Experiment::TermsOptIn',
- 'start',
- label: anything,
- property: 'experimental_group'
- )
-
- subject
- end
- end
- end
end
describe '#create' do
@@ -353,100 +313,26 @@
end
end
- context 'when terms are enforced' do
- before do
- enforce_terms
- end
-
- it 'redirects back with a notice when the checkbox was not checked' do
- subject
-
- expect(flash[:alert]).to eq(_('You must accept our Terms of Service and privacy policy in order to register an account'))
- end
-
- it 'creates the user with agreement when terms are accepted' do
- post :create, params: user_params.merge(terms_opt_in: '1')
-
- expect(controller.current_user).to be_present
- expect(controller.current_user.terms_accepted?).to be(true)
- end
-
- context 'when experiment terms_opt_in is enabled' do
+ context 'terms of service' do
+ context 'when terms are enforced' do
before do
- stub_experiment(terms_opt_in: true)
+ enforce_terms
end
- context 'when user is part of the experiment' do
- before do
- stub_experiment_for_user(terms_opt_in: true)
- end
-
- it 'creates the user with accepted terms' do
- subject
-
- expect(controller.current_user).to be_present
- expect(controller.current_user.terms_accepted?).to be(true)
- end
- end
-
- context 'when user is not part of the experiment' do
- before do
- stub_experiment_for_user(terms_opt_in: false)
- end
-
- it 'creates the user without accepted terms' do
- subject
+ it 'creates the user with accepted terms' do
+ subject
- expect(flash[:alert]).to eq(_('You must accept our Terms of Service and privacy policy in order to register an account'))
- end
+ expect(controller.current_user).to be_present
+ expect(controller.current_user.terms_accepted?).to be(true)
end
end
- end
-
- describe 'tracking data' do
- context 'with sign up flow and terms_opt_in experiment being enabled' do
- before do
- stub_experiment(signup_flow: true, terms_opt_in: true)
- end
-
- it 'records user for the terms_opt_in experiment' do
- expect(controller).to receive(:record_experiment_user).with(:terms_opt_in)
+ context 'when terms are not enforced' do
+ it 'creates the user without accepted terms' do
subject
- end
-
- context 'when user is not part of the experiment' do
- before do
- stub_experiment_for_user(signup_flow: true, terms_opt_in: false)
- end
-
- it 'tracks event with right parameters' do
- expect(Gitlab::Tracking).to receive(:event).with(
- 'Growth::Acquisition::Experiment::TermsOptIn',
- 'end',
- label: anything,
- property: 'control_group'
- )
-
- subject
- end
- end
-
- context 'when user is part of the experiment' do
- before do
- stub_experiment_for_user(signup_flow: true, terms_opt_in: true)
- end
-
- it 'tracks event with right parameters' do
- expect(Gitlab::Tracking).to receive(:event).with(
- 'Growth::Acquisition::Experiment::TermsOptIn',
- 'end',
- label: anything,
- property: 'experimental_group'
- )
- subject
- end
+ expect(controller.current_user).to be_present
+ expect(controller.current_user.terms_accepted?).to be(false)
end
end
end
diff --git a/spec/features/users/signup_spec.rb b/spec/features/users/signup_spec.rb
index 0ede19810d9ff..c59121626f034 100644
--- a/spec/features/users/signup_spec.rb
+++ b/spec/features/users/signup_spec.rb
@@ -222,22 +222,13 @@ def fill_in_signup_form
enforce_terms
end
- it 'requires the user to check the checkbox' do
+ it 'renders text that the user confirms terms by clicking register' do
visit new_user_registration_path
- fill_in_signup_form
- click_button 'Register'
-
- expect(current_path).to eq new_user_session_path
- expect(page).to have_content(/you must accept our terms of service/i)
- end
-
- it 'asks the user to accept terms before going to the dashboard' do
- visit new_user_registration_path
+ expect(page).to have_content(/By clicking Register, I agree that I have read and accepted the Terms of Use and Privacy Policy/)
fill_in_signup_form
- check :terms_opt_in
- click_button "Register"
+ click_button 'Register'
expect(current_path).to eq users_sign_up_welcome_path
end
@@ -364,28 +355,4 @@ def fill_in_signup_form
it_behaves_like 'Signup'
it_behaves_like 'Signup name validation', 'new_user_first_name', 127, 'First name'
it_behaves_like 'Signup name validation', 'new_user_last_name', 127, 'Last name'
-
- context 'when terms_opt_in experimental is enabled' do
- include TermsHelper
-
- before do
- enforce_terms
- stub_experiment(signup_flow: true, terms_opt_in: true)
- stub_experiment_for_user(signup_flow: true, terms_opt_in: true)
- end
-
- it 'terms are checked by default' 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
- click_button 'Register'
-
- expect(current_path).to eq users_sign_up_welcome_path
- end
- end
end
--
GitLab