Merge branch 'nicolasdular/remove-tos-checkobx' into 'master'

Remove accept terms checkbox for signup

See merge request gitlab-org/gitlab!42581

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 @@ class RegistrationsController < Devise::RegistrationsController
     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,11 +81,9 @@ class RegistrationsController < Devise::RegistrationsController
     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
-  end
 
   def set_role_required(new_user)
     new_user.set_role_required! if new_user.persisted?
@@ -185,18 +178,6 @@ class RegistrationsController < Devise::RegistrationsController
     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 @@ class RegistrationsController < Devise::RegistrationsController
     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

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'

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'

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

+- 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 }

changelogs/unreleased/nicolasdular-remove-tos-checkobx.yml

+---
+title: Remove accept terms checkbox for signup
+merge_request: 42581
+author:
+type: changed

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'

ee/spec/features/trial_registrations/signup_spec.rb

@@ -37,8 +37,6 @@ RSpec.describe 'Trial Sign Up', :js do
           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
 

lib/gitlab/experimentation.rb

@@ -51,9 +51,6 @@ module Gitlab
       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'
       },

locale/gitlab.pot

@@ -4431,7 +4431,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."
@@ -13283,9 +13283,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 ""
 
@@ -29782,9 +29779,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 ""
 

qa/qa/page/main/sign_up.rb

@@ -11,7 +11,6 @@ module QA
           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 @@ module QA
           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)
 

spec/controllers/registrations_controller_spec.rb

@@ -37,46 +37,6 @@ RSpec.describe RegistrationsController do
         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,34 +313,12 @@ RSpec.describe RegistrationsController do
       end
     end
 
+    context 'terms of service' do
       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
-        before do
-          stub_experiment(terms_opt_in: true)
-        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
 
@@ -389,64 +327,12 @@ RSpec.describe RegistrationsController do
         end
       end
 
-        context 'when user is not part of the experiment' do
-          before do
-            stub_experiment_for_user(terms_opt_in: false)
-          end
-
+      context 'when terms are not enforced' do
         it 'creates the user without 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
-        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)
-
-          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

spec/features/users/signup_spec.rb

@@ -222,22 +222,13 @@ RSpec.shared_examples 'Signup' do
       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 @@ RSpec.describe 'With experimental flow' do
   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