Store application wide terms

app/helpers/application_settings_helper.rb

@@ -248,7 +248,9 @@ module ApplicationSettingsHelper
       :user_default_external,
       :user_oauth_applications,
       :version_check_enabled,
-      :allow_local_requests_from_hooks_and_services
+      :allow_local_requests_from_hooks_and_services,
+      :enforce_terms,
+      :terms
     ]
   end
 end

app/models/application_setting.rb

@@ -220,12 +220,15 @@ class ApplicationSetting < ActiveRecord::Base
     end
   end
 
+  validate :terms_exist, if: :enforce_terms?
+
   before_validation :ensure_uuid!
 
   before_save :ensure_runners_registration_token
   before_save :ensure_health_check_access_token
 
   after_commit do
+    reset_memoized_terms
     Rails.cache.write(CACHE_KEY, self)
   end
 
@@ -507,6 +510,16 @@ class ApplicationSetting < ActiveRecord::Base
     password_authentication_enabled_for_web? || password_authentication_enabled_for_git?
   end
 
+  delegate :terms, to: :latest_terms, allow_nil: true
+  def latest_terms
+    @latest_terms ||= Term.latest
+  end
+
+  def reset_memoized_terms
+    @latest_terms = nil
+    latest_terms
+  end
+
   private
 
   def ensure_uuid!
@@ -520,4 +533,10 @@ class ApplicationSetting < ActiveRecord::Base
     errors.add(:repository_storages, "can't include: #{invalid.join(", ")}") unless
       invalid.empty?
   end
+
+  def terms_exist
+    return unless enforce_terms?
+
+    errors.add(:terms, "You need to set terms to be enforced") unless terms.present?
+  end
 end

app/models/application_setting/term.rb

 class ApplicationSetting
   class Term < ActiveRecord::Base
+    include CacheMarkdownField
+
     validates :terms, presence: true
+
+    cache_markdown_field :terms
+
+    def self.latest
+      order(:id).last
+    end
   end
 end

app/services/application_settings/update_service.rb

 module ApplicationSettings
   class UpdateService < ApplicationSettings::BaseService
     def execute
+      update_terms(@params.delete(:terms))
+
       @application_setting.update(@params)
     end
+
+    private
+
+    def update_terms(terms)
+      return unless terms.present?
+
+      # Avoid creating a new terms record if the text is exactly the same.
+      terms = terms.strip
+      return if terms == @application_setting.terms
+
+      ApplicationSetting::Term.create(terms: terms)
+      @application_setting.reset_memoized_terms
+    end
   end
 end

app/views/admin/application_settings/_terms.html.haml

@@ -9,7 +9,7 @@
             = f.check_box :enforce_terms
             = _("Require all users to accept Terms of Service when they access GitLab.")
           .help-block
-            When enabled, users cannot use GitLab until the terms have been accepted.
+            = _("When enabled, users cannot use GitLab until the terms have been accepted.")
     .form-group
       .col-sm-12
         = f.label :terms do
@@ -17,6 +17,6 @@
       .col-sm-12
         = f.text_area :terms, class: 'form-control', rows: 8
         .help-block
-          Markdown enabled
+          = _("Markdown enabled")
 
-  = f.submit 'Save changes', class: "btn btn-success"
+  = f.submit _("Save changes"), class: "btn btn-success"

app/views/admin/application_settings/show.html.haml

@@ -47,6 +47,17 @@
   .settings-content
     = render 'signin'
 
+%section.settings.as-terms.no-animate#js-terms-settings{ class: ('expanded' if expanded) }
+  .settings-header
+    %h4
+      = _('Terms of Service')
+    %button.btn.js-settings-toggle{ type: 'button' }
+      = expanded ? 'Collapse' : 'Expand'
+    %p
+      = _('Include a Terms of Service agreement that all users must accept.')
+  .settings-content
+    = render 'terms'
+
 %section.settings.as-help-page.no-animate#js-help-settings{ class: ('expanded' if expanded) }
   .settings-header
     %h4

doc/api/settings.md

@@ -53,6 +53,8 @@ Example response:
    "dsa_key_restriction": 0,
    "ecdsa_key_restriction": 0,
    "ed25519_key_restriction": 0,
+   "enforce_terms": true,
+   "terms": "Hello world!",
 }
 ```
 
@@ -153,6 +155,8 @@ PUT /application/settings
 | `user_default_external`                  | boolean          | no                                            | Newly registered users will by default be external                                                                                                                                                                                                                                                                                                                                                                                                        |
 | `user_oauth_applications`                | boolean          | no                                            | Allow users to register any application to use GitLab as an OAuth provider                                                                                                                                                                                                                                                                                                                                                                                |
 | `version_check_enabled`                  | boolean          | no                                            | Let GitLab inform you when an update is available.                                                                                                                                                                                                                                                                                                                                                                                                        |
+| `enforce_terms`                          | boolean          | no                                            | Enforce application ToS to all users                                                                                                                                                                                                                                                                                                                                                                                                         |
+| `terms`                                  | text             | yes (if `enforce_terms` is true)              | Markdown content for the ToS                                                                                                                                                                                                                                                                                                                                                                                                                 |
 
 ```bash
 curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/application/settings?signup_enabled=false&default_project_visibility=internal
@@ -195,5 +199,7 @@ Example response:
   "dsa_key_restriction": 0,
   "ecdsa_key_restriction": 0,
   "ed25519_key_restriction": 0,
+  "enforce_terms": true,
+  "terms": "Hello world!",
 }
 ```

spec/factories/terms.rb

+FactoryBot.define do
+  factory :term, class: ApplicationSetting::Term do
+    terms "Lorem ipsum dolor sit amet, consectetur adipiscing elit."
+  end
+end

spec/features/admin/admin_settings_spec.rb

@@ -85,6 +85,18 @@ feature 'Admin updates settings' do
     expect(page).to have_content "Application settings saved successfully"
   end
 
+  scenario 'Terms of Service' do
+    page.within('.as-terms') do
+      check 'Require all users to accept Terms of Service when they access GitLab.'
+      fill_in 'Terms of Service Agreement', with: 'Be nice!'
+      click_button 'Save changes'
+    end
+
+    expect(Gitlab::CurrentSettings.enforce_terms).to be(true)
+    expect(Gitlab::CurrentSettings.terms).to eq 'Be nice!'
+    expect(page).to have_content 'Application settings saved successfully'
+  end
+
   scenario 'Modify oauth providers' do
     expect(Gitlab::CurrentSettings.disabled_oauth_sign_in_sources).to be_empty
 

spec/models/application_setting/term_spec.rb

+require 'spec_helper'
+
+describe ApplicationSetting::Term do
+  describe 'validations' do
+    it { is_expected.to validate_presence_of(:terms) }
+  end
+
+  describe '.latest' do
+    it 'finds the latest terms' do
+      terms = create(:term)
+
+      expect(described_class.latest).to eq(terms)
+    end
+  end
+end

spec/models/application_setting_spec.rb

@@ -301,6 +301,21 @@ describe ApplicationSetting do
         expect(subject).to be_invalid
       end
     end
+
+    describe 'enforcing terms' do
+      it 'requires the terms to present when enforcing users to accept' do
+        subject.enforce_terms = true
+
+        expect(subject).to be_invalid
+      end
+
+      it 'is valid when terms are created' do
+        create(:term)
+        subject.enforce_terms = true
+
+        expect(subject).to be_valid
+      end
+    end
   end
 
   describe '.current' do

spec/requests/api/settings_spec.rb

@@ -54,7 +54,9 @@ describe API::Settings, 'Settings' do
           dsa_key_restriction: 2048,
           ecdsa_key_restriction: 384,
           ed25519_key_restriction: 256,
-          circuitbreaker_check_interval: 2
+          circuitbreaker_check_interval: 2,
+          enforce_terms: true,
+          terms: 'Hello world!'
 
         expect(response).to have_gitlab_http_status(200)
         expect(json_response['default_projects_limit']).to eq(3)
@@ -76,6 +78,8 @@ describe API::Settings, 'Settings' do
         expect(json_response['ecdsa_key_restriction']).to eq(384)
         expect(json_response['ed25519_key_restriction']).to eq(256)
         expect(json_response['circuitbreaker_check_interval']).to eq(2)
+        expect(json_response['enforce_terms']).to be(true)
+        expect(json_response['terms']).to eq('Hello world!')
       end
     end
 

spec/services/application_settings/update_service_spec.rb

+require 'spec_helper'
+
+describe ApplicationSettings::UpdateService do
+  let(:application_settings) { Gitlab::CurrentSettings.current_application_settings }
+  let(:admin) { create(:user, :admin) }
+  let(:params) { {} }
+
+  subject { described_class.new(application_settings, admin, params) }
+
+  before do
+    # So the caching behaves like it would in production
+    stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
+  end
+
+  describe 'updating terms' do
+    context 'when the passed terms are blank' do
+      let(:params) { { terms: ''  } }
+
+      it 'does not create terms' do
+        expect { subject.execute }.not_to change { ApplicationSetting::Term.count }
+      end
+    end
+
+    context 'when passing terms' do
+      let(:params) { { terms: 'Be nice!  '  } }
+
+      it 'creates the terms' do
+        expect { subject.execute }.to change { ApplicationSetting::Term.count }.by(1)
+      end
+
+      it 'does not create terms if they are the same as the existing ones' do
+        create(:term, terms: 'Be nice!')
+
+        expect { subject.execute }.not_to change { ApplicationSetting::Term.count }
+      end
+
+      it 'updates terms if they already existed' do
+        create(:term, terms: 'Other terms')
+
+        subject.execute
+
+        expect(application_settings.terms).to eq('Be nice!')
+      end
+
+      it 'Only queries once when the terms are changed' do
+        create(:term, terms: 'Other terms')
+        expect(application_settings.terms).to eq('Other terms')
+
+        subject.execute
+
+        expect(application_settings.terms).to eq('Be nice!')
+        expect { 2.times { application_settings.terms } }
+          .not_to exceed_query_limit(0)
+      end
+    end
+  end
+end