Add latest changes from gitlab-org/gitlab@13-5-stable-eev13.5.0-rc42

4 files changed, 114 insertions, 110 deletions

diff --git a/spec/controllers/concerns/controller_with_feature_category/config_spec.rb b/spec/controllers/concerns/controller_with_feature_category/config_spec.rb
deleted file mode 100644
index 9b8ffd2baab..00000000000
--- a/spec/controllers/concerns/controller_with_feature_category/config_spec.rb
+++ /dev/null
@@ -1,53 +0,0 @@
-# frozen_string_literal: true
-
-require "fast_spec_helper"
-require "rspec-parameterized"
-require_relative "../../../../app/controllers/concerns/controller_with_feature_category/config"
-
-RSpec.describe ControllerWithFeatureCategory::Config do
-  describe "#matches?" do
-    using RSpec::Parameterized::TableSyntax
-
-    where(:only_actions, :except_actions, :if_proc, :unless_proc, :test_action, :expected) do
-      nil         | nil         | nil   | nil   | "action"   | true
-      [:included] | nil         | nil   | nil   | "action"   | false
-      [:included] | nil         | nil   | nil   | "included" | true
-      nil         | [:excluded] | nil   | nil   | "excluded" | false
-      nil         | nil         | true  | nil   | "action"   | true
-      [:included] | nil         | true  | nil   | "action"   | false
-      [:included] | nil         | true  | nil   | "included" | true
-      nil         | [:excluded] | true  | nil   | "excluded" | false
-      nil         | nil         | false | nil   | "action"   | false
-      [:included] | nil         | false | nil   | "action"   | false
-      [:included] | nil         | false | nil   | "included" | false
-      nil         | [:excluded] | false | nil   | "excluded" | false
-      nil         | nil         | nil   | true  | "action"   | false
-      [:included] | nil         | nil   | true  | "action"   | false
-      [:included] | nil         | nil   | true  | "included" | false
-      nil         | [:excluded] | nil   | true  | "excluded" | false
-      nil         | nil         | nil   | false | "action"   | true
-      [:included] | nil         | nil   | false | "action"   | false
-      [:included] | nil         | nil   | false | "included" | true
-      nil         | [:excluded] | nil   | false | "excluded" | false
-      nil         | nil         | true  | false | "action"   | true
-      [:included] | nil         | true  | false | "action"   | false
-      [:included] | nil         | true  | false | "included" | true
-      nil         | [:excluded] | true  | false | "excluded" | false
-      nil         | nil         | false | true  | "action"   | false
-      [:included] | nil         | false | true  | "action"   | false
-      [:included] | nil         | false | true  | "included" | false
-      nil         | [:excluded] | false | true  | "excluded" | false
-    end
-
-    with_them do
-      let(:config) do
-        if_to_proc = if_proc.nil? ? nil : -> (_) { if_proc }
-        unless_to_proc = unless_proc.nil? ? nil : -> (_) { unless_proc }
-
-        described_class.new(:category, only_actions, except_actions, if_to_proc, unless_to_proc)
-      end
-
-      specify { expect(config.matches?(test_action)).to be(expected) }
-    end
-  end
-end
diff --git a/spec/controllers/concerns/controller_with_feature_category_spec.rb b/spec/controllers/concerns/controller_with_feature_category_spec.rb
index e603a7d14c4..55e84755f5c 100644
--- a/spec/controllers/concerns/controller_with_feature_category_spec.rb
+++ b/spec/controllers/concerns/controller_with_feature_category_spec.rb
@@ -2,7 +2,6 @@
 
 require 'fast_spec_helper'
 require_relative "../../../app/controllers/concerns/controller_with_feature_category"
-require_relative "../../../app/controllers/concerns/controller_with_feature_category/config"
 
 RSpec.describe ControllerWithFeatureCategory do
   describe ".feature_category_for_action" do
@@ -14,17 +13,15 @@ RSpec.describe ControllerWithFeatureCategory do
 
     let(:controller) do
       Class.new(base_controller) do
-        feature_category :baz
-        feature_category :foo, except: %w(update edit)
-        feature_category :bar, only: %w(index show)
-        feature_category :quux, only: %w(destroy)
-        feature_category :quuz, only: %w(destroy)
+        feature_category :foo, %w(update edit)
+        feature_category :bar, %w(index show)
+        feature_category :quux, %w(destroy)
       end
     end
 
     let(:subclass) do
       Class.new(controller) do
-        feature_category :qux, only: %w(index)
+        feature_category :baz, %w(subclass_index)
       end
     end
 
@@ -33,34 +30,31 @@ RSpec.describe ControllerWithFeatureCategory do
     end
 
     it "returns the expected category", :aggregate_failures do
-      expect(controller.feature_category_for_action("update")).to eq(:baz)
-      expect(controller.feature_category_for_action("hello")).to eq(:foo)
+      expect(controller.feature_category_for_action("update")).to eq(:foo)
       expect(controller.feature_category_for_action("index")).to eq(:bar)
+      expect(controller.feature_category_for_action("destroy")).to eq(:quux)
     end
 
-    it "returns the closest match for categories defined in subclasses" do
-      expect(subclass.feature_category_for_action("index")).to eq(:qux)
-      expect(subclass.feature_category_for_action("show")).to eq(:bar)
+    it "returns the expected category for categories defined in subclasses" do
+      expect(subclass.feature_category_for_action("subclass_index")).to eq(:baz)
     end
 
-    it "returns the last defined feature category when multiple match" do
-      expect(controller.feature_category_for_action("destroy")).to eq(:quuz)
-    end
-
-    it "raises an error when using including and excluding the same action" do
+    it "raises an error when defining for the controller and for individual actions" do
       expect do
         Class.new(base_controller) do
-          feature_category :hello, only: [:world], except: [:world]
+          feature_category :hello
+          feature_category :goodbye, [:world]
         end
-      end.to raise_error(%r(cannot configure both `only` and `except`))
+      end.to raise_error(ArgumentError, "hello is defined for all actions, but other categories are set")
     end
 
-    it "raises an error when using unknown arguments" do
+    it "raises an error when multiple calls define the same action" do
       expect do
         Class.new(base_controller) do
-          feature_category :hello, hello: :world
+          feature_category :hello, [:world]
+          feature_category :goodbye, ["world"]
         end
-      end.to raise_error(%r(unknown arguments))
+      end.to raise_error(ArgumentError, "Actions have multiple feature categories: world")
     end
   end
 end
diff --git a/spec/controllers/concerns/issuable_collections_spec.rb b/spec/controllers/concerns/issuable_collections_spec.rb
index befdd760965..6fa273bf3d7 100644
--- a/spec/controllers/concerns/issuable_collections_spec.rb
+++ b/spec/controllers/concerns/issuable_collections_spec.rb
@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe IssuableCollections do
+  using RSpec::Parameterized::TableSyntax
+
   let(:user) { create(:user) }
 
   let(:controller) do
@@ -25,13 +27,35 @@ RSpec.describe IssuableCollections do
   end
 
   describe '#page_count_for_relation' do
-    let(:params) { { state: 'opened' } }
+    let(:relation) { double(:relation, limit_value: 20) }
+
+    context 'row count is known' do
+      let(:params) { { state: 'opened' } }
+
+      it 'returns the number of pages' do
+        pages = controller.send(:page_count_for_relation, relation, 28)
+
+        expect(pages).to eq(2)
+      end
+    end
+
+    context 'row_count is unknown' do
+      where(:page_param, :expected) do
+        nil | 2
+        1   | 2
+        '1' | 2
+        2   | 3
+      end
 
-    it 'returns the number of pages' do
-      relation = double(:relation, limit_value: 20)
-      pages = controller.send(:page_count_for_relation, relation, 28)
+      with_them do
+        let(:params) { { state: 'opened', page: page_param } }
 
-      expect(pages).to eq(2)
+        it 'returns current page + 1 if the row count is unknown' do
+          pages = controller.send(:page_count_for_relation, relation, -1)
+
+          expect(pages).to eq(expected)
+        end
+      end
     end
   end
 
diff --git a/spec/controllers/concerns/redis_tracking_spec.rb b/spec/controllers/concerns/redis_tracking_spec.rb
index 3795fca5576..831f5ad7bb1 100644
--- a/spec/controllers/concerns/redis_tracking_spec.rb
+++ b/spec/controllers/concerns/redis_tracking_spec.rb
@@ -3,15 +3,19 @@
 require "spec_helper"
 
 RSpec.describe RedisTracking do
-  let(:event_name) { 'g_compliance_dashboard' }
-  let(:feature) { 'g_compliance_dashboard_feature' }
+  let(:feature) { 'approval_rule' }
   let(:user) { create(:user) }
 
+  before do
+    skip_feature_flags_yaml_validation
+  end
+
   controller(ApplicationController) do
     include RedisTracking
 
     skip_before_action :authenticate_user!, only: :show
-    track_redis_hll_event :index, :show, name: 'i_analytics_dev_ops_score', feature: :g_compliance_dashboard_feature, feature_default_enabled: true
+    track_redis_hll_event :index, :show, name: 'g_compliance_approval_rules', feature: :approval_rule, feature_default_enabled: true,
+      if: [:custom_condition_one?, :custom_condition_two?]
 
     def index
       render html: 'index'
@@ -24,51 +28,94 @@ RSpec.describe RedisTracking do
     def show
       render html: 'show'
     end
-  end
 
-  context 'with feature disabled' do
-    it 'does not track the event' do
-      stub_feature_flags(feature => false)
+    private
 
-      expect(Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event)
+    def custom_condition_one?
+      true
+    end
 
-      get :index
+    def custom_condition_two?
+      true
     end
   end
 
-  context 'with usage ping disabled' do
+  def expect_tracking
+    expect(Gitlab::UsageDataCounters::HLLRedisCounter).to receive(:track_event)
+      .with(instance_of(String), 'g_compliance_approval_rules')
+  end
+
+  def expect_no_tracking
+    expect(Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event)
+  end
+
+  context 'with feature disabled' do
     it 'does not track the event' do
-      stub_feature_flags(feature => true)
-      allow(Gitlab::CurrentSettings).to receive(:usage_ping_enabled?).and_return(false)
+      stub_feature_flags(feature => false)
 
-      expect(Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event)
+      expect_no_tracking
 
       get :index
     end
   end
 
-  context 'with feature enabled and usage ping enabled' do
+  context 'with feature enabled' do
     before do
       stub_feature_flags(feature => true)
-      allow(Gitlab::CurrentSettings).to receive(:usage_ping_enabled?).and_return(true)
     end
 
     context 'when user is logged in' do
-      it 'tracks the event' do
+      before do
         sign_in(user)
+      end
 
-        expect(Gitlab::UsageDataCounters::HLLRedisCounter).to receive(:track_event)
+      it 'tracks the event' do
+        expect_tracking
 
         get :index
       end
 
       it 'passes default_enabled flag' do
-        sign_in(user)
-
         expect(controller).to receive(:metric_feature_enabled?).with(feature.to_sym, true)
 
         get :index
       end
+
+      it 'tracks the event if DNT is not enabled' do
+        request.headers['DNT'] = '0'
+
+        expect_tracking
+
+        get :index
+      end
+
+      it 'does not track the event if DNT is enabled' do
+        request.headers['DNT'] = '1'
+
+        expect_no_tracking
+
+        get :index
+      end
+
+      it 'does not track the event if the format is not HTML' do
+        expect_no_tracking
+
+        get :index, format: :json
+      end
+
+      it 'does not track the event if a custom condition returns false' do
+        expect(controller).to receive(:custom_condition_two?).and_return(false)
+
+        expect_no_tracking
+
+        get :index
+      end
+
+      it 'does not track the event for untracked actions' do
+        expect_no_tracking
+
+        get :new
+      end
     end
 
     context 'when user is not logged in and there is a visitor_id' do
@@ -81,26 +128,18 @@ RSpec.describe RedisTracking do
       it 'tracks the event' do
         cookies[:visitor_id] = { value: visitor_id, expires: 24.months }
 
-        expect(Gitlab::UsageDataCounters::HLLRedisCounter).to receive(:track_event)
+        expect_tracking
 
         get :show
       end
     end
 
     context 'when user is not logged in and there is no visitor_id' do
-      it 'does not tracks the event' do
-        expect(Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event)
+      it 'does not track the event' do
+        expect_no_tracking
 
         get :index
       end
     end
-
-    context 'for untracked action' do
-      it 'does not tracks the event' do
-        expect(Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event)
-
-        get :new
-      end
-    end
   end
 end