From 285ffb223896f2226531be2ba10933414194155c Mon Sep 17 00:00:00 2001 From: tauriedavis Date: Fri, 25 May 2018 16:17:57 -0700 Subject: [PATCH] Messaging on terms page when user already accepted We show a blue flash banner if the user already accepted, and show a button allowing them to continue to the application. --- app/controllers/users/terms_controller.rb | 4 ++ app/models/application_setting/term.rb | 6 +++ app/models/term_agreement.rb | 2 + app/views/users/terms/index.html.haml | 4 ++ .../46585-gdpr-terms-acceptance.yml | 6 +++ .../users/terms_controller_spec.rb | 22 +++++++++-- spec/factories/term_agreements.rb | 8 ++++ spec/features/users/terms_spec.rb | 16 ++++++++ spec/models/application_setting/term_spec.rb | 37 +++++++++++++++++++ spec/models/term_agreement_spec.rb | 9 +++++ 10 files changed, 111 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/46585-gdpr-terms-acceptance.yml diff --git a/app/controllers/users/terms_controller.rb b/app/controllers/users/terms_controller.rb index ab685b9106e..f7c6d1d59db 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 e8ce0ccbb71..3b1dfe7e4ef 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 8458a231bbd..c317bd0c90b 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 e0fe551cf36..79dd55c53cc 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 00000000000..84853846b0e --- /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 a744463413c..0d77e91a67d 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 557599e663d..3c4eebd0196 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 1efa5cd5490..af407c52917 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 1eddf3c56ff..aa49594f4d1 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 a59bf119692..950dfa09a6a 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 -- GitLab