Merge branch 'usage_consent' into 'master'

Ask user explicitly about usage stats agreement

Closes #48962

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

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 || '/';

app/assets/javascripts/usage_ping_consent.js

+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.');
+      });
+  });
+};

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;

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,

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

app/helpers/version_check_helper.rb

 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

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
 

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),

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?
 

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,

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"

app/views/shared/_ping_consent.html.haml

+- 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'

changelogs/unreleased/usage_consent.yml

+---
+title: Ask user explicitly about usage stats agreement on single user deployments.
+merge_request: 21423
+author:
+type: added

db/migrate/20180906101639_add_user_ping_consent_to_application_settings.rb

+# 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

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

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 ""
 

spec/features/usage_stats_consent_spec.rb

+# 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

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) }