Merge branch '46585-gdpr-terms-acceptance' into 'master'

Resolve "Missing Accept button for terms of service"

Closes #46585

See merge request gitlab-org/gitlab-ce!19156

app/controllers/users/terms_controller.rb

@@ -13,6 +13,10 @@ module Users
 
     def index
       @redirect = redirect_path
+
+      if @term.accepted_by_user?(current_user)
+        flash.now[:notice] = "You have already accepted the Terms of Service as #{current_user.to_reference}"
+      end
     end
 
     def accept

app/models/application_setting/term.rb

 class ApplicationSetting
   class Term < ActiveRecord::Base
     include CacheMarkdownField
+    has_many :term_agreements
 
     validates :terms, presence: true
 
@@ -9,5 +10,10 @@ class ApplicationSetting
     def self.latest
       order(:id).last
     end
+
+    def accepted_by_user?(user)
+      user.accepted_term_id == id ||
+        term_agreements.accepted.where(user: user).exists?
+    end
   end
 end

app/models/term_agreement.rb

@@ -2,5 +2,7 @@ class TermAgreement < ActiveRecord::Base
   belongs_to :term, class_name: 'ApplicationSetting::Term'
   belongs_to :user
 
+  scope :accepted, -> { where(accepted: true) }
+
   validates :user, :term, presence: true
 end

app/views/users/terms/index.html.haml

@@ -7,6 +7,10 @@
     .float-right
       = button_to accept_term_path(@term, redirect_params), class: 'btn btn-success prepend-left-8' do
         = _('Accept terms')
+  - else
+    .pull-right
+      = link_to root_path, class: 'btn btn-success prepend-left-8' do
+        = _('Continue')
   - if can?(current_user, :decline_terms, @term)
     .float-right
       = button_to decline_term_path(@term, redirect_params), class: 'btn btn-default prepend-left-8' do

changelogs/unreleased/46585-gdpr-terms-acceptance.yml

+---
+title: Add flash notice if user has already accepted terms and allow users to continue
+  to root path
+merge_request: 19156
+author:
+type: changed

spec/controllers/users/terms_controller_spec.rb

 require 'spec_helper'
 
 describe Users::TermsController do
+  include TermsHelper
   let(:user) { create(:user) }
   let(:term) { create(:term) }
 
@@ -15,10 +16,25 @@ describe Users::TermsController do
       expect(response).to have_gitlab_http_status(:redirect)
     end
 
-    it 'shows terms when they exist' do
-      term
+    context 'when terms exist' do
+      before do
+        stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
+        term
+      end
+
+      it 'shows terms when they exist' do
+        get :index
+
+        expect(response).to have_gitlab_http_status(:success)
+      end
 
-      expect(response).to have_gitlab_http_status(:success)
+      it 'shows a message when the user already accepted the terms' do
+        accept_terms(user)
+
+        get :index
+
+        expect(controller).to set_flash.now[:notice].to(/already accepted/)
+      end
     end
   end
 

spec/factories/term_agreements.rb

@@ -3,4 +3,12 @@ FactoryBot.define do
     term
     user
   end
+
+  trait :declined do
+    accepted false
+  end
+
+  trait :accepted do
+    accepted true
+  end
 end

spec/features/users/terms_spec.rb

@@ -39,6 +39,22 @@ describe 'Users > Terms' do
     end
   end
 
+  context 'when the user has already accepted the terms' do
+    before do
+      accept_terms(user)
+    end
+
+    it 'allows the user to continue to the app' do
+      visit terms_path
+
+      expect(page).to have_content "You have already accepted the Terms of Service as #{user.to_reference}"
+
+      click_link 'Continue'
+
+      expect(current_path).to eq(root_path)
+    end
+  end
+
   context 'terms were enforced while session is active', :js do
     let(:project) { create(:project) }
 

spec/models/application_setting/term_spec.rb

@@ -12,4 +12,41 @@ describe ApplicationSetting::Term do
       expect(described_class.latest).to eq(terms)
     end
   end
+
+  describe '#accepted_by_user?' do
+    let(:user) { create(:user) }
+    let(:term) { create(:term) }
+
+    it 'is true when the user accepted the terms' do
+      accept_terms(term, user)
+
+      expect(term.accepted_by_user?(user)).to be(true)
+    end
+
+    it 'is false when the user declined the terms' do
+      decline_terms(term, user)
+
+      expect(term.accepted_by_user?(user)).to be(false)
+    end
+
+    it 'does not cause a query when the user accepted the current terms' do
+      accept_terms(term, user)
+
+      expect { term.accepted_by_user?(user) }.not_to exceed_query_limit(0)
+    end
+
+    it 'returns false if the currently accepted terms are different' do
+      accept_terms(create(:term), user)
+
+      expect(term.accepted_by_user?(user)).to be(false)
+    end
+
+    def accept_terms(term, user)
+      Users::RespondToTermsService.new(user, term).execute(accepted: true)
+    end
+
+    def decline_terms(term, user)
+      Users::RespondToTermsService.new(user, term).execute(accepted: false)
+    end
+  end
 end

spec/models/term_agreement_spec.rb

@@ -5,4 +5,13 @@ describe TermAgreement do
     it { is_expected.to validate_presence_of(:term) }
     it { is_expected.to validate_presence_of(:user) }
   end
+
+  describe '.accepted' do
+    it 'only includes accepted terms' do
+      accepted = create(:term_agreement, :accepted)
+      create(:term_agreement, :declined)
+
+      expect(described_class.accepted).to contain_exactly(accepted)
+    end
+  end
 end