From d23c1f5f675ce9850e9107ba273568d3acced6f1 Mon Sep 17 00:00:00 2001 From: Niko Belokolodov Date: Thu, 11 Sep 2025 12:02:39 +1200 Subject: [PATCH 1/2] Detect if FE tracking parameters are declared --- danger/analytics_instrumentation/Dangerfile | 2 + .../danger/analytics_instrumentation_spec.rb | 112 ++++++++++++++++++ tooling/danger/analytics_instrumentation.rb | 50 ++++++++ 3 files changed, 164 insertions(+) diff --git a/danger/analytics_instrumentation/Dangerfile b/danger/analytics_instrumentation/Dangerfile index 81a9f7abf85be7..fe771090724631 100644 --- a/danger/analytics_instrumentation/Dangerfile +++ b/danger/analytics_instrumentation/Dangerfile @@ -17,3 +17,5 @@ analytics_instrumentation.warn_about_migrated_redis_keys_specs! analytics_instrumentation.warn_about_potential_pii_tracking analytics_instrumentation.prohibit_key_path_changes! + +analytics_instrumentation.verify_fe_tracking_params diff --git a/spec/tooling/danger/analytics_instrumentation_spec.rb b/spec/tooling/danger/analytics_instrumentation_spec.rb index 693e30da717e5f..76205191e3dc3c 100644 --- a/spec/tooling/danger/analytics_instrumentation_spec.rb +++ b/spec/tooling/danger/analytics_instrumentation_spec.rb @@ -673,4 +673,116 @@ end end end + + describe '#verify_fe_tracking_params' do + subject(:verify_fe_tracking_params) { analytics_instrumentation.verify_fe_tracking_params } + + let(:js_files) { %w[app/assets/javascripts/test.js app/assets/javascripts/component.vue] } + + before do + allow(fake_helper).to receive(:all_changed_files).and_return(js_files) + allow(analytics_instrumentation).to receive(:project_helper).and_return(fake_project_helper) + allow(fake_project_helper).to receive(:file_lines).and_return([]) + + js_files.each do |file| + allow(fake_helper).to receive(:changed_lines).with(file).and_return(file_changes[file] || []) + end + end + + shared_context 'with event definition' do + before do + allow(File).to receive(:exist?).and_return(false) + allow(File).to receive(:exist?).with("config/events/#{event_name}.yml").and_return(ce_file_exists) + allow(File).to receive(:exist?).with("ee/config/events/#{event_name}.yml").and_return(ee_file_exists) + + if ce_file_exists + + allow(YAML).to receive(:load_file).with("config/events/#{event_name}.yml").and_return(event_definition) + elsif ee_file_exists + + allow(YAML).to receive(:load_file).with("ee/config/events/#{event_name}.yml").and_return(event_definition) + end + end + end + + shared_examples 'does not suggest changes' do + it 'does not add suggestions' do + expect(analytics_instrumentation).not_to receive(:add_suggestion) + + verify_fe_tracking_params + end + end + + shared_examples 'suggests property changes' do + it 'adds suggestion for missing properties' do + expect(analytics_instrumentation).to receive(:add_suggestion) + + verify_fe_tracking_params + end + end + + context 'when no trackEvent calls are added' do + let(:file_changes) { { 'app/assets/javascripts/test.js' => ['+ console.log("new code");'] } } + + it_behaves_like 'does not suggest changes' + end + + context 'when trackEvent call has valid properties' do + include_context 'with event definition' + + let(:event_name) { 'valid_event' } + let(:ce_file_exists) { true } + let(:ee_file_exists) { false } + let(:event_definition) { { 'additional_properties' => { 'action' => {}, 'source' => {} } } } + let(:file_changes) { { 'app/assets/javascripts/test.js' => ["+ this.trackEvent('#{event_name}', { action: 'click', source: 'button' });"] } } + + it_behaves_like 'does not suggest changes' + end + + context 'when trackEvent call has missing properties' do + include_context 'with event definition' + + let(:event_name) { 'invalid_event' } + let(:ce_file_exists) { true } + let(:ee_file_exists) { false } + let(:event_definition) { { 'additional_properties' => { 'action' => {} } } } + let(:file_changes) { { 'app/assets/javascripts/test.js' => ["+ this.trackEvent('#{event_name}', { action: 'click', missing_prop: 'value' });"] } } + + it_behaves_like 'suggests property changes' + end + + context 'when trackEvent call with EE event definition' do + include_context 'with event definition' + + let(:event_name) { 'ee_event' } + let(:ce_file_exists) { false } + let(:ee_file_exists) { true } + let(:event_definition) { { 'additional_properties' => { 'feature' => {} } } } + let(:file_changes) { { 'app/assets/javascripts/test.js' => ["+ this.trackEvent('#{event_name}', { feature: 'advanced', missing: 'prop' });"] } } + + it_behaves_like 'suggests property changes' + end + + context 'when multi-line trackEvent call' do + include_context 'with event definition' + + let(:event_name) { 'multiline_event' } + let(:ce_file_exists) { true } + let(:ee_file_exists) { false } + let(:event_definition) { { 'additional_properties' => { 'label' => {} } } } + let(:file_changes) do + { + 'app/assets/javascripts/component.vue' => [ + "+ this.trackEvent('#{event_name}',", + "+ {", + "+ label: 'test',", + "+ invalid_prop: 'value'", + "+ });" + ] + } + end + + it_behaves_like 'suggests property changes' + end + end end diff --git a/tooling/danger/analytics_instrumentation.rb b/tooling/danger/analytics_instrumentation.rb index c50ebe9d12bbef..3de5801fc6ff2a 100644 --- a/tooling/danger/analytics_instrumentation.rb +++ b/tooling/danger/analytics_instrumentation.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true +require 'yaml' require_relative 'suggestor' module Tooling @@ -146,6 +147,55 @@ def prohibit_key_path_changes! end end + def verify_fe_tracking_params + js_files = helper.all_changed_files.select { |filename| filename.end_with?(".js", ".vue") } + + js_files.each do |filename| + changes = helper.changed_lines(filename) + + # Join all added lines, remove + prefix and clean whitespace to simplify searching + added_content = changes + .select { |line| line.start_with?('+') } + .map { |line| line[1..] } + .join('') + .gsub(/\s+/, ' ') + .strip + + # Find all trackEvent calls in the simplified content + added_content.scan(/trackEvent\(['"]([^'"]+)['"](?:,\s*\{([^}]+)\})?/) do |event_name, hash_content| + tracked_properties = hash_content ? hash_content.scan(/(\w+):/).flatten : [] + + # Find event definition file and extract additional_properties + def_folders = %w[config/events ee/config/events] + event_file_path = nil + + def_folders.each do |folder| + potential_path = File.join(folder, "#{event_name}.yml") + if File.exist?(potential_path) + event_file_path = potential_path + break + end + end + + if event_file_path + event_definition = YAML.load_file(event_file_path) + additional_properties = event_definition['additional_properties'] || {} + + # Check if additional_properties contains all tracked_properties + additional_property_keys = additional_properties.keys + missing_keys = tracked_properties - additional_property_keys + unless missing_keys.empty? + add_suggestion( + filename: filename, + regex: /#{event_name}/, + comment_text: "Tracked properties #{missing_keys} are not defined in additional_properties in #{event_file_path}\n Please add the missing properties to the event definition." + ) + end + end + end + end + end + private def modified_config_files -- GitLab From 2083c553fe69a6a1b48698ee6aa750f03de3dda7 Mon Sep 17 00:00:00 2001 From: Niko Belokolodov Date: Thu, 11 Sep 2025 14:42:03 +1200 Subject: [PATCH 2/2] Verify new Danger FE checks --- .../ai/components/explore_duo_core_banner.vue | 12 +++++++++--- .../components/projects_report/projects_table.vue | 1 + .../projects/get_started/components/get_started.vue | 1 + 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/ee/app/assets/javascripts/ai/components/explore_duo_core_banner.vue b/ee/app/assets/javascripts/ai/components/explore_duo_core_banner.vue index 6b5c27ff06841d..24ace9098c8beb 100644 --- a/ee/app/assets/javascripts/ai/components/explore_duo_core_banner.vue +++ b/ee/app/assets/javascripts/ai/components/explore_duo_core_banner.vue @@ -30,14 +30,20 @@ export default { exploreGitLabDuoLink: `${DOCS_URL_IN_EE_DIR}/user/gitlab_duo/#summary-of-gitlab-duo-features`, ctaLink: `${DOCS_URL_IN_EE_DIR}/user/get_started/getting_started_gitlab_duo/`, mounted() { - this.trackEvent('render_duo_core_banner'); + this.trackEvent('render_duo_core_banner', { custom: 'custom_value' }); }, methods: { onInstallExtensionClick() { - this.trackEvent('click_extension_link_on_duo_core_banner'); + this.trackEvent('click_extension_link_on_duo_core_banner', + { custom: 'custom_value' } + ); }, onExploreGitLabDuoClick() { - this.trackEvent('click_explore_link_on_duo_core_banner'); + this.trackEvent('click_explore_link_on_duo_core_banner', + { + custom: 'custom_value', + property: 'property' + }); }, handlePrimaryAction(dismiss) { this.trackEvent('click_cta_link_on_duo_core_banner'); diff --git a/ee/app/assets/javascripts/compliance_dashboard/components/projects_report/projects_table.vue b/ee/app/assets/javascripts/compliance_dashboard/components/projects_report/projects_table.vue index 47a254d31b8ac2..a174c123a5062e 100644 --- a/ee/app/assets/javascripts/compliance_dashboard/components/projects_report/projects_table.vue +++ b/ee/app/assets/javascripts/compliance_dashboard/components/projects_report/projects_table.vue @@ -189,6 +189,7 @@ export default { this.trackEvent('apply_compliance_framework', { property: operations.map((entry) => entry.projectId).join(','), + not_exists: 'custom' }); if (isBulkAction) { diff --git a/ee/app/assets/javascripts/pages/projects/get_started/components/get_started.vue b/ee/app/assets/javascripts/pages/projects/get_started/components/get_started.vue index 10d7e67242ddb7..04b7b2335ea0cd 100644 --- a/ee/app/assets/javascripts/pages/projects/get_started/components/get_started.vue +++ b/ee/app/assets/javascripts/pages/projects/get_started/components/get_started.vue @@ -104,6 +104,7 @@ export default { label: 'get_started', property: 'progress_percentage_on_end', value: this.completionPercentage, + not_exists: 'value' }); visitUrl(this.tutorialEndPath); -- GitLab