diff --git a/app/controllers/users/terms_controller.rb b/app/controllers/users/terms_controller.rb index ab685b9106efb6c363e426d2e76b138f51e3e1d3..f7c6d1d59dba40b91428eb65aedbede8f91d6ab5 100644 --- a/app/controllers/users/terms_controller.rb +++ b/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 diff --git a/app/models/application_setting/term.rb b/app/models/application_setting/term.rb index e8ce0ccbb719ffefa66ade7974493cea25bd5fb5..3b1dfe7e4efda73bb9f025754feeeb81a2ccfcfc 100644 --- a/app/models/application_setting/term.rb +++ b/app/models/application_setting/term.rb @@ -1,6 +1,7 @@ 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 diff --git a/app/models/term_agreement.rb b/app/models/term_agreement.rb index 8458a231bbddd3321c08b3e2e9bcec015f46b548..c317bd0c90be40c9338789ebe14743eefa81549a 100644 --- a/app/models/term_agreement.rb +++ b/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 diff --git a/app/views/users/terms/index.html.haml b/app/views/users/terms/index.html.haml index e0fe551cf36b2c7a19971c07d9d651dec4d18cbf..79dd55c53ccc265bfafcae6186b460210dd6e737 100644 --- a/app/views/users/terms/index.html.haml +++ b/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 diff --git a/changelogs/unreleased/46585-gdpr-terms-acceptance.yml b/changelogs/unreleased/46585-gdpr-terms-acceptance.yml new file mode 100644 index 0000000000000000000000000000000000000000..84853846b0e532eb3af555d05b4934afd0dfe84f --- /dev/null +++ b/changelogs/unreleased/46585-gdpr-terms-acceptance.yml @@ -0,0 +1,6 @@ +--- +title: Add flash notice if user has already accepted terms and allow users to continue + to root path +merge_request: 19156 +author: +type: changed diff --git a/spec/controllers/users/terms_controller_spec.rb b/spec/controllers/users/terms_controller_spec.rb index a744463413cf3134558e4c7af637794b316f77bc..0d77e91a67d07f0847e5bb28e3d3dcb41f4d8a53 100644 --- a/spec/controllers/users/terms_controller_spec.rb +++ b/spec/controllers/users/terms_controller_spec.rb @@ -1,6 +1,7 @@ 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 diff --git a/spec/factories/term_agreements.rb b/spec/factories/term_agreements.rb index 557599e663d2996542c72bd678e37dc12218fce3..3c4eebd0196810a2b6b09d43c77c7c98f2d78b7c 100644 --- a/spec/factories/term_agreements.rb +++ b/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 diff --git a/spec/features/users/terms_spec.rb b/spec/features/users/terms_spec.rb index 1efa5cd549067de0b67b92eb9c7c2be13998c09d..af407c52917dc8de9cf4e0cea0bad2baeae2eeaf 100644 --- a/spec/features/users/terms_spec.rb +++ b/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) } diff --git a/spec/models/application_setting/term_spec.rb b/spec/models/application_setting/term_spec.rb index 1eddf3c56ff563af200ddea4bfca6158807487ef..aa49594f4d12dcb01f5966eaffe7f5f7da30f214 100644 --- a/spec/models/application_setting/term_spec.rb +++ b/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 diff --git a/spec/models/term_agreement_spec.rb b/spec/models/term_agreement_spec.rb index a59bf1196926374b1f0c17a93bd33c99cf72e6f7..950dfa09a6a68843652db924a467d071e9b02add 100644 --- a/spec/models/term_agreement_spec.rb +++ b/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