Merge branch...

Merge branch '341754-follow-up-from-skip-product-intelligence-review-for-growth-experiments-mrs' into 'master'

Move ProductIntelligence Danger code to project_helper class

See merge request gitlab-org/gitlab!72544

danger/product_intelligence/Dangerfile

@@ -12,12 +12,12 @@ For MR review guidelines, see the [Service Ping review guidelines](https://docs.
 MSG
 
 # exit if not matching files or if no product intelligence labels
-matching_changed_files = product_intelligence.matching_changed_files
+product_intelligence_paths_to_review = project_helper.changes_by_category[:product_intelligence]
 labels = product_intelligence.missing_labels
 
-return if matching_changed_files.empty? || labels.empty?
+return if product_intelligence_paths_to_review.empty? || labels.empty?
 
-warn format(CHANGED_FILES_MESSAGE, changed_files: helper.markdown_list(matching_changed_files))
+warn format(CHANGED_FILES_MESSAGE, changed_files: helper.markdown_list(product_intelligence_paths_to_review))
 
 gitlab.api.update_merge_request(gitlab.mr_json['project_id'],
                                     gitlab.mr_json['iid'],

spec/tooling/danger/product_intelligence_spec.rb

@@ -68,71 +68,4 @@ RSpec.describe Tooling::Danger::ProductIntelligence do
       it { is_expected.to be_empty }
     end
   end
-
-  describe '#matching_changed_files' do
-    subject { product_intelligence.matching_changed_files }
-
-    let(:changed_files) do
-      [
-        'dashboard/todos_controller.rb',
-        'components/welcome.vue',
-        'admin/groups/_form.html.haml'
-      ]
-    end
-
-    context 'with snowplow files changed' do
-      context 'when vue file changed' do
-        let(:changed_lines) { ['+data-track-action'] }
-
-        it { is_expected.to match_array(['components/welcome.vue']) }
-      end
-
-      context 'when haml file changed' do
-        let(:changed_lines) { ['+ data: { track_label:'] }
-
-        it { is_expected.to match_array(['admin/groups/_form.html.haml']) }
-      end
-
-      context 'when ruby file changed' do
-        let(:changed_lines) { ['+ Gitlab::Tracking.event'] }
-        let(:changed_files) { ['dashboard/todos_controller.rb', 'admin/groups/_form.html.haml'] }
-
-        it { is_expected.to match_array(['dashboard/todos_controller.rb']) }
-      end
-    end
-
-    context 'with metrics files changed' do
-      let(:changed_files) { ['config/metrics/counts_7d/test_metric.yml', 'ee/config/metrics/counts_7d/ee_metric.yml'] }
-
-      it { is_expected.to match_array(changed_files) }
-    end
-
-    context 'with metrics files not changed' do
-      it { is_expected.to be_empty }
-    end
-
-    context 'with tracking files changed' do
-      let(:changed_files) do
-        [
-          'lib/gitlab/tracking.rb',
-          'spec/lib/gitlab/tracking_spec.rb',
-          'app/helpers/tracking_helper.rb'
-        ]
-      end
-
-      it { is_expected.to match_array(changed_files) }
-    end
-
-    context 'with usage_data files changed' do
-      let(:changed_files) do
-        [
-          'doc/api/usage_data.md',
-          'ee/lib/ee/gitlab/usage_data.rb',
-          'spec/lib/gitlab/usage_data_spec.rb'
-        ]
-      end
-
-      it { is_expected.to match_array(changed_files) }
-    end
-  end
 end

spec/tooling/danger/project_helper_spec.rb

@@ -40,7 +40,7 @@ RSpec.describe Tooling::Danger::ProjectHelper do
     using RSpec::Parameterized::TableSyntax
 
     before do
-      allow(fake_git).to receive(:diff_for_file).with('usage_data.rb') { double(:diff, patch: "+ count(User.active)") }
+      allow(fake_git).to receive(:diff_for_file).with(instance_of(String)) { double(:diff, patch: "+ count(User.active)") }
     end
 
     where(:path, :expected_categories) do
@@ -189,6 +189,10 @@ RSpec.describe Tooling::Danger::ProjectHelper do
       'spec/frontend/tracking/foo.js' | [:frontend, :product_intelligence]
       'spec/frontend/tracking_spec.js' | [:frontend, :product_intelligence]
       'lib/gitlab/usage_database/foo.rb' | [:backend]
+      'config/metrics/counts_7d/test_metric.yml' | [:product_intelligence]
+      'config/metrics/schema.json' | [:product_intelligence]
+      'doc/api/usage_data.md' | [:product_intelligence]
+      'spec/lib/gitlab/usage_data_spec.rb' | [:product_intelligence]
     end
 
     with_them do
@@ -199,6 +203,9 @@ RSpec.describe Tooling::Danger::ProjectHelper do
 
     context 'having specific changes' do
       where(:expected_categories, :patch, :changed_files) do
+        [:product_intelligence]                      | '+data-track-action'                           | ['components/welcome.vue']
+        [:product_intelligence]                      | '+ data: { track_label:'                       | ['admin/groups/_form.html.haml']
+        [:product_intelligence]                      | '+ Gitlab::Tracking.event'                     | ['dashboard/todos_controller.rb', 'admin/groups/_form.html.haml']
         [:database, :backend, :product_intelligence] | '+ count(User.active)'                         | ['usage_data.rb', 'lib/gitlab/usage_data.rb', 'ee/lib/ee/gitlab/usage_data.rb']
         [:database, :backend, :product_intelligence] | '+ estimate_batch_distinct_count(User.active)' | ['usage_data.rb']
         [:backend, :product_intelligence]            | '+ alt_usage_data(User.active)'                | ['lib/gitlab/usage_data.rb']

tooling/danger/product_intelligence.rb

@@ -9,26 +9,6 @@ module Tooling
         'product intelligence::review pending'
       ].freeze
 
-      TRACKING_FILES = [
-        'lib/gitlab/tracking.rb',
-        'spec/lib/gitlab/tracking_spec.rb',
-        'app/helpers/tracking_helper.rb',
-        'spec/helpers/tracking_helper_spec.rb',
-        'app/assets/javascripts/tracking/index.js',
-        'app/assets/javascripts/tracking/constants.js',
-        'app/assets/javascripts/tracking/get_standard_context.js',
-        'spec/frontend/tracking/get_standard_context_spec.js',
-        'spec/frontend/tracking_spec.js',
-        'generator_templates/usage_metric_definition/metric_definition.yml',
-        'lib/generators/gitlab/usage_metric/usage_metric_generator.rb',
-        'lib/generators/gitlab/usage_metric_definition_generator.rb',
-        'lib/generators/gitlab/usage_metric_definition/redis_hll_generator.rb',
-        'spec/lib/generators/gitlab/usage_metric_generator_spec.rb',
-        'spec/lib/generators/gitlab/usage_metric_definition_generator_spec.rb',
-        'spec/lib/generators/gitlab/usage_metric_definition/redis_hll_generator_spec.rb',
-        'config/metrics/schema.json'
-      ].freeze
-
       def missing_labels
         return [] if !helper.ci? || helper.mr_has_labels?('growth experiment')
 
@@ -38,43 +18,6 @@ module Tooling
 
         labels
       end
-
-      def matching_changed_files
-        tracking_changed_files = all_changed_files & TRACKING_FILES
-        usage_data_changed_files = all_changed_files.grep(%r{(usage_data)})
-
-        usage_data_changed_files + tracking_changed_files + metrics_changed_files + snowplow_changed_files
-      end
-
-      private
-
-      def all_changed_files
-        helper.all_changed_files
-      end
-
-      def metrics_changed_files
-        all_changed_files.grep(%r{((ee/)?config/metrics/.*\.yml)})
-      end
-
-      def matching_files?(file, extension:, pattern:)
-        return unless file.end_with?(extension)
-
-        helper.changed_lines(file).grep(pattern).any?
-      end
-
-      def snowplow_changed_files
-        js_patterns = Regexp.union(
-          'Tracking.event',
-          /\btrack\(/,
-          'data-track-action'
-        )
-        all_changed_files.select do |file|
-          matching_files?(file, extension: '.rb', pattern: %r{Gitlab::Tracking\.(event|enabled\?|options)$}) ||
-            matching_files?(file, extension: '.js', pattern: js_patterns) ||
-            matching_files?(file, extension: '.vue', pattern: js_patterns) ||
-            matching_files?(file, extension: '.haml', pattern: %r{data: \{ track})
-        end
-      end
     end
   end
 end

tooling/danger/project_helper.rb

@@ -38,6 +38,8 @@ module Tooling
 
         %r{\A((ee|jh)/)?config/feature_flags/} => :feature_flag,
 
+        %r{doc/api/usage_data.md} => [:product_intelligence],
+
         %r{\Adoc/.*(\.(md|png|gif|jpg|yml))\z} => :docs,
         %r{\A(CONTRIBUTING|LICENSE|MAINTENANCE|PHILOSOPHY|PROCESS|README)(\.md)?\z} => :docs,
         %r{\Adata/whats_new/} => :docs,
@@ -100,6 +102,7 @@ module Tooling
         %r{\A((ee|jh)/)?spec/support/shared_contexts/features/} => :test,
         %r{\A((ee|jh)/)?spec/support/helpers/features/} => :test,
 
+        %r{\A((spec/)?lib/generators/gitlab/usage_metric_)} => [:product_intelligence],
         %r{\A((ee|jh)/)?lib/gitlab/usage_data_counters/.*\.yml\z} => [:product_intelligence],
         %r{\A((ee|jh)/)?config/metrics/((.*\.yml)|(schema\.json))\z} => [:product_intelligence],
         %r{\A((ee|jh)/)?lib/gitlab/usage_data(_counters)?(/|\.rb)} => [:backend, :product_intelligence],
@@ -108,9 +111,16 @@ module Tooling
           spec/lib/gitlab/tracking_spec\.rb |
           app/helpers/tracking_helper\.rb |
           spec/helpers/tracking_helper_spec\.rb |
+          (spec/)?lib/generators/gitlab/usage_metric_\S+ |
+          (spec/)?lib/generators/gitlab/usage_metric_definition/redis_hll_generator(_spec)?\.rb |
           lib/generators/rails/usage_metric_definition_generator\.rb |
           spec/lib/generators/usage_metric_definition_generator_spec\.rb |
           generator_templates/usage_metric_definition/metric_definition\.yml)\z}x => [:backend, :product_intelligence],
+        %r{gitlab/usage_data(_spec)?\.rb} => [:product_intelligence],
+        [%r{\.haml\z}, %r{data: \{ track}] => [:product_intelligence],
+        [%r{\.(rb|haml)\z}, %r{Gitlab::Tracking\.(event|enabled\?|options)$}] => [:product_intelligence],
+        [%r{\.(vue|js)\z}, %r{(Tracking.event|/\btrack\(/|data-track-action)}] => [:product_intelligence],
+
         %r{\A((ee|jh)/)?app/(?!assets|views)[^/]+} => :backend,
         %r{\A((ee|jh)/)?(bin|config|generator_templates|lib|rubocop)/} => :backend,
         %r{\A((ee|jh)/)?spec/migrations} => :database,