diff options
-rw-r--r-- | app/assets/javascripts/main.js | 2 | ||||
-rw-r--r-- | app/assets/javascripts/usage_ping_consent.js | 30 | ||||
-rw-r--r-- | app/assets/stylesheets/framework/layout.scss | 19 | ||||
-rw-r--r-- | app/controllers/admin/application_settings_controller.rb | 24 | ||||
-rw-r--r-- | app/controllers/application_controller.rb | 26 | ||||
-rw-r--r-- | app/helpers/version_check_helper.rb | 10 | ||||
-rw-r--r-- | app/models/application_setting.rb | 3 | ||||
-rw-r--r-- | app/models/user.rb | 22 | ||||
-rw-r--r-- | app/services/application_settings/update_service.rb | 8 | ||||
-rw-r--r-- | app/services/submit_usage_ping_service.rb | 1 | ||||
-rw-r--r-- | app/views/layouts/_page.html.haml | 1 | ||||
-rw-r--r-- | app/views/shared/_ping_consent.html.haml | 12 | ||||
-rw-r--r-- | changelogs/unreleased/usage_consent.yml | 5 | ||||
-rw-r--r-- | db/migrate/20180906101639_add_user_ping_consent_to_application_settings.rb | 19 | ||||
-rw-r--r-- | db/schema.rb | 4 | ||||
-rw-r--r-- | locale/gitlab.pot | 9 | ||||
-rw-r--r-- | spec/features/usage_stats_consent_spec.rb | 45 | ||||
-rw-r--r-- | spec/models/user_spec.rb | 42 |
18 files changed, 267 insertions, 15 deletions
diff --git a/app/assets/javascripts/main.js b/app/assets/javascripts/main.js index 2718f73a830..c5a5f64abac 100644 --- a/app/assets/javascripts/main.js +++ b/app/assets/javascripts/main.js @@ -29,6 +29,7 @@ import './milestone_select'; import './frequent_items'; import initBreadcrumbs from './breadcrumb'; import initDispatcher from './dispatcher'; +import initUsagePingConsent from './usage_ping_consent'; // expose jQuery as global (TODO: remove these) window.jQuery = jQuery; @@ -78,6 +79,7 @@ document.addEventListener('DOMContentLoaded', () => { initImporterStatus(); initTodoToggle(); initLogoAnimation(); + initUsagePingConsent(); // Set the default path for all cookies to GitLab's root directory Cookies.defaults.path = gon.relative_url_root || '/'; diff --git a/app/assets/javascripts/usage_ping_consent.js b/app/assets/javascripts/usage_ping_consent.js new file mode 100644 index 00000000000..ae3fde190e3 --- /dev/null +++ b/app/assets/javascripts/usage_ping_consent.js @@ -0,0 +1,30 @@ +import $ from 'jquery'; +import axios from './lib/utils/axios_utils'; +import Flash, { hideFlash } from './flash'; +import { convertPermissionToBoolean } from './lib/utils/common_utils'; + +export default () => { + $('body').on('click', '.js-usage-consent-action', (e) => { + e.preventDefault(); + e.stopImmediatePropagation(); // overwrite rails listener + + const { url, checkEnabled, pingEnabled } = e.target.dataset; + const data = { + application_setting: { + version_check_enabled: convertPermissionToBoolean(checkEnabled), + usage_ping_enabled: convertPermissionToBoolean(pingEnabled), + }, + }; + + const hideConsentMessage = () => hideFlash(document.querySelector('.ping-consent-message')); + + axios.put(url, data) + .then(() => { + hideConsentMessage(); + }) + .catch(() => { + hideConsentMessage(); + Flash('Something went wrong. Try again later.'); + }); + }); +}; diff --git a/app/assets/stylesheets/framework/layout.scss b/app/assets/stylesheets/framework/layout.scss index d4bae4cb137..9218df9b40f 100644 --- a/app/assets/stylesheets/framework/layout.scss +++ b/app/assets/stylesheets/framework/layout.scss @@ -69,10 +69,14 @@ body { float: right; } - /* Center alert text and alert action links on smaller screens */ - @include media-breakpoint-down(sm) { - .alert { - text-align: center; + .flex-alert { + @include media-breakpoint-up(lg) { + display: flex; + + .alert-message { + flex: 1; + padding-right: 40px; + } } .alert-link-group { @@ -80,6 +84,13 @@ body { } } + @include media-breakpoint-down(sm) { + .alert-link-group { + float: none; + margin-top: $gl-padding-8; + } + } + /* Stripe the background colors so that adjacent alert-warnings are distinct from one another */ .alert-warning { transition: background-color 0.15s, border-color 0.15s; diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 9723e400574..869213d61f1 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -9,11 +9,18 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController .new(@application_setting, current_user, application_setting_params) .execute - if successful - redirect_to admin_application_settings_path, - notice: 'Application settings saved successfully' - else - render :show + if recheck_user_consent? + session[:ask_for_usage_stats_consent] = current_user.requires_usage_stats_consent? + end + + respond_to do |format| + if successful + format.json { head :ok } + format.html { redirect_to admin_application_settings_path, notice: 'Application settings saved successfully' } + else + format.json { head :bad_request } + format.html { render :show } + end end end @@ -76,6 +83,13 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController ) end + def recheck_user_consent? + return false unless session[:ask_for_usage_stats_consent] + return false unless params[:application_setting] + + params[:application_setting].key?(:usage_ping_enabled) || params[:application_setting].key?(:version_check_enabled) + end + def visible_application_setting_attributes ApplicationSettingsHelper.visible_attributes + [ :domain_blacklist_file, diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 7cd68d6b92a..7e2b2cf3ad3 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -22,6 +22,7 @@ class ApplicationController < ActionController::Base before_action :add_gon_variables, unless: [:peek_request?, :json_request?] before_action :configure_permitted_parameters, if: :devise_controller? before_action :require_email, unless: :devise_controller? + before_action :set_usage_stats_consent_flag around_action :set_locale @@ -434,4 +435,29 @@ class ApplicationController < ActionController::Base !(peek_request? || devise_controller?) end + + def set_usage_stats_consent_flag + return unless current_user + return if sessionless_user? + return if session.has_key?(:ask_for_usage_stats_consent) + + session[:ask_for_usage_stats_consent] = current_user.requires_usage_stats_consent? + + if session[:ask_for_usage_stats_consent] + disable_usage_stats + end + end + + def disable_usage_stats + application_setting_params = { + usage_ping_enabled: false, + version_check_enabled: false, + skip_usage_stats_user: true + } + settings = Gitlab::CurrentSettings.current_application_settings + + ApplicationSettings::UpdateService + .new(settings, current_user, application_setting_params) + .execute + end end diff --git a/app/helpers/version_check_helper.rb b/app/helpers/version_check_helper.rb index c20753ece72..a7fef93e7f1 100644 --- a/app/helpers/version_check_helper.rb +++ b/app/helpers/version_check_helper.rb @@ -1,8 +1,10 @@ module VersionCheckHelper def version_status_badge - if Rails.env.production? && Gitlab::CurrentSettings.version_check_enabled - image_url = VersionCheck.new.url - image_tag image_url, class: 'js-version-status-badge' - end + return unless Rails.env.production? + return unless Gitlab::CurrentSettings.version_check_enabled + return if User.single_user&.requires_usage_stats_consent? + + image_url = VersionCheck.new.url + image_tag image_url, class: 'js-version-status-badge' end end diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 03bd7fa016e..d8536c5512d 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -302,7 +302,8 @@ class ApplicationSetting < ActiveRecord::Base instance_statistics_visibility_private: false, user_default_external: false, user_default_internal_regex: nil, - user_show_add_ssh_key_message: true + user_show_add_ssh_key_message: true, + usage_stats_set_by_user_id: nil } end diff --git a/app/models/user.rb b/app/models/user.rb index 0fcc952b5cd..568ec101016 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -490,6 +490,16 @@ class User < ActiveRecord::Base u.name = 'Ghost User' end end + + # Return true if there is only single non-internal user in the deployment, + # ghost user is ignored. + def single_user? + User.non_internal.limit(2).count == 1 + end + + def single_user + User.non_internal.first if single_user? + end end def full_path @@ -1287,6 +1297,10 @@ class User < ActiveRecord::Base !terms_accepted? end + def requires_usage_stats_consent? + !consented_usage_stats? && 7.days.ago > self.created_at && !has_current_license? && User.single_user? + end + # @deprecated alias_method :owned_or_masters_groups, :owned_or_maintainers_groups @@ -1301,6 +1315,14 @@ class User < ActiveRecord::Base private + def has_current_license? + false + end + + def consented_usage_stats? + Gitlab::CurrentSettings.usage_stats_set_by_user_id == self.id + end + def owned_projects_union Gitlab::SQL::Union.new([ Project.where(namespace: namespace), diff --git a/app/services/application_settings/update_service.rb b/app/services/application_settings/update_service.rb index 19cf34e2ac4..2e4643ed668 100644 --- a/app/services/application_settings/update_service.rb +++ b/app/services/application_settings/update_service.rb @@ -11,11 +11,19 @@ module ApplicationSettings params[:performance_bar_allowed_group_id] = performance_bar_allowed_group_id end + if usage_stats_updated? && !params.delete(:skip_usage_stats_user) + params[:usage_stats_set_by_user_id] = current_user.id + end + @application_setting.update(@params) end private + def usage_stats_updated? + params.key?(:usage_ping_enabled) || params.key?(:version_check_enabled) + end + def update_terms(terms) return unless terms.present? diff --git a/app/services/submit_usage_ping_service.rb b/app/services/submit_usage_ping_service.rb index 93c2e222963..62222d3fd2a 100644 --- a/app/services/submit_usage_ping_service.rb +++ b/app/services/submit_usage_ping_service.rb @@ -15,6 +15,7 @@ class SubmitUsagePingService def execute return false unless Gitlab::CurrentSettings.usage_ping_enabled? + return false if User.single_user&.requires_usage_stats_consent? response = Gitlab::HTTP.post( URL, diff --git a/app/views/layouts/_page.html.haml b/app/views/layouts/_page.html.haml index f67a8878c80..a41d30da450 100644 --- a/app/views/layouts/_page.html.haml +++ b/app/views/layouts/_page.html.haml @@ -8,6 +8,7 @@ = render "layouts/broadcast" = render 'layouts/header/read_only_banner' = yield :flash_message + = render "shared/ping_consent" - unless @hide_breadcrumbs = render "layouts/nav/breadcrumbs" = render "layouts/flash" diff --git a/app/views/shared/_ping_consent.html.haml b/app/views/shared/_ping_consent.html.haml new file mode 100644 index 00000000000..f8eb2b2833b --- /dev/null +++ b/app/views/shared/_ping_consent.html.haml @@ -0,0 +1,12 @@ +- if session[:ask_for_usage_stats_consent] + .ping-consent-message.alert.alert-warning.flex-alert + - settings_link_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer" class="alert-link">'.html_safe % { url: admin_application_settings_path(anchor: 'js-usage-settings') } + - info_link_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer" class="alert-link">'.html_safe % { url: help_page_path('user/admin_area/settings/usage_statistics.md') } + .alert-message + = s_('To help improve GitLab, we would like to periodically collect usage information. This can be changed at any time in %{settings_link_start}Settings%{link_end}. %{info_link_start}More Information%{link_end}').html_safe % { settings_link_start: settings_link_start, info_link_start: info_link_start, link_end: '</a>'.html_safe } + .alert-link-group + - send_usage_data_path = admin_application_settings_path(application_setting: { version_check_enabled: 1, usage_ping_enabled: 1 }) + - not_now_path = admin_application_settings_path(application_setting: { version_check_enabled: 0, usage_ping_enabled: 0 }) + = link_to _("Send usage data"), send_usage_data_path, 'data-url' => admin_application_settings_path, method: :put, 'data-check-enabled': true, 'data-ping-enabled': true, class: 'alert-link js-usage-consent-action' + | + = link_to _('Not now'), not_now_path, 'data-url' => admin_application_settings_path, method: :put, 'data-check-enabled': false, 'data-ping-enabled': false, class: 'hide-ping-consent-message alert-link js-usage-consent-action' diff --git a/changelogs/unreleased/usage_consent.yml b/changelogs/unreleased/usage_consent.yml new file mode 100644 index 00000000000..56e00879b9a --- /dev/null +++ b/changelogs/unreleased/usage_consent.yml @@ -0,0 +1,5 @@ +--- +title: Ask user explicitly about usage stats agreement on single user deployments. +merge_request: 21423 +author: +type: added diff --git a/db/migrate/20180906101639_add_user_ping_consent_to_application_settings.rb b/db/migrate/20180906101639_add_user_ping_consent_to_application_settings.rb new file mode 100644 index 00000000000..5d0e67d2648 --- /dev/null +++ b/db/migrate/20180906101639_add_user_ping_consent_to_application_settings.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class AddUserPingConsentToApplicationSettings < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column :application_settings, :usage_stats_set_by_user_id, :integer + add_concurrent_foreign_key :application_settings, :users, column: :usage_stats_set_by_user_id, on_delete: :nullify + end + + def down + remove_foreign_key :application_settings, column: :usage_stats_set_by_user_id + remove_column :application_settings, :usage_stats_set_by_user_id + end +end diff --git a/db/schema.rb b/db/schema.rb index e0b704f82d4..a417d0bc70a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180901200537) do +ActiveRecord::Schema.define(version: 20180906101639) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -172,6 +172,7 @@ ActiveRecord::Schema.define(version: 20180901200537) do t.boolean "instance_statistics_visibility_private", default: false, null: false t.boolean "web_ide_clientside_preview_enabled", default: false, null: false t.boolean "user_show_add_ssh_key_message", default: true, null: false + t.integer "usage_stats_set_by_user_id" end create_table "audit_events", force: :cascade do |t| @@ -2275,6 +2276,7 @@ ActiveRecord::Schema.define(version: 20180901200537) do add_index "web_hooks", ["project_id"], name: "index_web_hooks_on_project_id", using: :btree add_index "web_hooks", ["type"], name: "index_web_hooks_on_type", using: :btree + add_foreign_key "application_settings", "users", column: "usage_stats_set_by_user_id", name: "fk_964370041d", on_delete: :nullify add_foreign_key "badges", "namespaces", column: "group_id", on_delete: :cascade add_foreign_key "badges", "projects", on_delete: :cascade add_foreign_key "boards", "namespaces", column: "group_id", on_delete: :cascade diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 491da5d9631..8b19e2bec61 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -4004,6 +4004,9 @@ msgstr "" msgid "Not enough data" msgstr "" +msgid "Not now" +msgstr "" + msgid "Note that the master branch is automatically protected. %{link_to_protected_branches}" msgstr "" @@ -5177,6 +5180,9 @@ msgstr "" msgid "Send email" msgstr "" +msgid "Send usage data" +msgstr "" + msgid "Sep" msgstr "" @@ -6145,6 +6151,9 @@ msgstr "" msgid "To help improve GitLab and its user experience, GitLab will periodically collect usage information." msgstr "" +msgid "To help improve GitLab, we would like to periodically collect usage information. This can be changed at any time in %{settings_link_start}Settings%{link_end}. %{info_link_start}More Information%{link_end}" +msgstr "" + msgid "To import GitHub repositories, you can use a %{personal_access_token_link}. When you create your Personal Access Token, you will need to select the <code>repo</code> scope, so we can display a list of your public and private repositories which are available to import." msgstr "" diff --git a/spec/features/usage_stats_consent_spec.rb b/spec/features/usage_stats_consent_spec.rb new file mode 100644 index 00000000000..dd8f3179895 --- /dev/null +++ b/spec/features/usage_stats_consent_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'Usage stats consent' do + context 'when signed in' do + let(:user) { create(:admin, created_at: 8.days.ago) } + let(:message) { 'To help improve GitLab, we would like to periodically collect usage information.' } + + before do + allow(user).to receive(:has_current_license?).and_return false + + gitlab_sign_in(user) + end + + it 'hides the banner permanently when sets usage stats' do + visit root_dashboard_path + + expect(page).to have_content(message) + + click_link 'Send usage data' + + expect(page).not_to have_content(message) + expect(page).to have_content('Application settings saved successfully') + + gitlab_sign_out + gitlab_sign_in(user) + visit root_dashboard_path + + expect(page).not_to have_content(message) + end + + it 'shows banner on next session if user did not set usage stats' do + visit root_dashboard_path + + expect(page).to have_content(message) + + gitlab_sign_out + gitlab_sign_in(user) + visit root_dashboard_path + + expect(page).to have_content(message) + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 2a7aff39240..bee4a3d24a7 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2957,6 +2957,48 @@ describe User do end end + describe '#requires_usage_stats_consent?' do + let(:user) { create(:user, created_at: 8.days.ago) } + + before do + allow(user).to receive(:has_current_license?).and_return false + end + + context 'in single-user environment' do + it 'requires user consent after one week' do + create(:user, ghost: true) + + expect(user.requires_usage_stats_consent?).to be true + end + + it 'requires user consent after one week if there is another ghost user' do + expect(user.requires_usage_stats_consent?).to be true + end + + it 'does not require consent in the first week' do + user.created_at = 6.days.ago + + expect(user.requires_usage_stats_consent?).to be false + end + + it 'does not require consent if usage stats were set by this user' do + allow(Gitlab::CurrentSettings).to receive(:usage_stats_set_by_user_id).and_return(user.id) + + expect(user.requires_usage_stats_consent?).to be false + end + end + + context 'in multi-user environment' do + before do + create(:user) + end + + it 'does not require consent' do + expect(user.requires_usage_stats_consent?).to be false + end + end + end + context 'with uploads' do it_behaves_like 'model with mounted uploader', false do let(:model_object) { create(:user, :with_avatar) } |