Merge branch '254660-review-usage-of-suggest_pipeline-feature-flag' into 'master'

Resolve suggest_pipeline feature flag naming issues

Closes #254660

See merge request gitlab-org/gitlab!42985

app/assets/javascripts/lib/utils/experimentation.js

+export function isExperimentEnabled(experimentKey) {
+  return Boolean(window.gon?.experiments?.[experimentKey]);
+}

app/assets/javascripts/pages/projects/blob/show/index.js

@@ -6,6 +6,7 @@ import GpgBadges from '~/gpg_badges';
 import '~/sourcegraph/load';
 import PipelineTourSuccessModal from '~/blob/pipeline_tour_success_modal.vue';
 import { parseBoolean } from '~/lib/utils/common_utils';
+import { isExperimentEnabled } from '~/lib/utils/experimentation';
 
 const createGitlabCiYmlVisualization = (containerId = '#js-blob-toggle-graph-preview') => {
   const el = document.querySelector(containerId);
@@ -70,7 +71,7 @@ document.addEventListener('DOMContentLoaded', () => {
     );
   }
 
-  if (gon.features?.suggestPipeline) {
+  if (isExperimentEnabled('suggestPipeline')) {
     const successPipelineEl = document.querySelector('.js-success-pipeline-modal');
 
     if (successPipelineEl) {

app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue

@@ -45,6 +45,7 @@ import GroupedTestReportsApp from '../reports/components/grouped_test_reports_ap
 import { setFaviconOverlay } from '../lib/utils/common_utils';
 import GroupedAccessibilityReportsApp from '../reports/accessibility_report/grouped_accessibility_reports_app.vue';
 import getStateQuery from './queries/get_state.query.graphql';
+import { isExperimentEnabled } from '~/lib/utils/experimentation';
 
 export default {
   el: '#js-vue-mr-widget',
@@ -148,7 +149,7 @@ export default {
     },
     shouldSuggestPipelines() {
       return (
-        gon.features?.suggestPipeline &&
+        isExperimentEnabled('suggestPipeline') &&
         !this.mr.hasCI &&
         this.mr.mergeRequestAddCiConfigPath &&
         !this.mr.isDismissedSuggestPipeline

app/controllers/projects/blob_controller.rb

@@ -33,7 +33,7 @@ class Projects::BlobController < Projects::ApplicationController
   before_action :set_last_commit_sha, only: [:edit, :update]
 
   before_action only: :show do
-    push_frontend_feature_flag(:suggest_pipeline) if experiment_enabled?(:suggest_pipeline)
+    push_frontend_experiment(:suggest_pipeline)
     push_frontend_feature_flag(:gitlab_ci_yml_preview, @project, default_enabled: false)
   end
 

app/controllers/projects/merge_requests_controller.rb

@@ -27,7 +27,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   before_action :authenticate_user!, only: [:assign_related_issues]
   before_action :check_user_can_push_to_source_branch!, only: [:rebase]
   before_action only: [:show] do
-    push_frontend_feature_flag(:suggest_pipeline) if experiment_enabled?(:suggest_pipeline)
+    push_frontend_experiment(:suggest_pipeline)
     push_frontend_feature_flag(:widget_visibility_polling, @project, default_enabled: true)
     push_frontend_feature_flag(:merge_ref_head_comments, @project, default_enabled: true)
     push_frontend_feature_flag(:mr_commit_neighbor_nav, @project, default_enabled: true)

app/helpers/suggest_pipeline_helper.rb

@@ -2,7 +2,7 @@
 
 module SuggestPipelineHelper
   def should_suggest_gitlab_ci_yml?
-    Feature.enabled?(:suggest_pipeline) &&
+    experiment_enabled?(:suggest_pipeline) &&
       current_user &&
       params[:suggest_gitlab_ci_yml] == 'true'
   end

app/serializers/merge_request_widget_entity.rb

@@ -67,15 +67,15 @@ class MergeRequestWidgetEntity < Grape::Entity
     )
   end
 
-  expose :user_callouts_path, if: -> (*) { Feature.enabled?(:suggest_pipeline) } do |merge_request|
+  expose :user_callouts_path, if: -> (*) { Gitlab::Experimentation.enabled?(:suggest_pipeline) } do |_merge_request|
     user_callouts_path
   end
 
-  expose :suggest_pipeline_feature_id, if: -> (*) { Feature.enabled?(:suggest_pipeline) } do |merge_request|
+  expose :suggest_pipeline_feature_id, if: -> (*) { Gitlab::Experimentation.enabled?(:suggest_pipeline) } do |_merge_request|
     SUGGEST_PIPELINE
   end
 
-  expose :is_dismissed_suggest_pipeline, if: -> (*) { Feature.enabled?(:suggest_pipeline) } do |merge_request|
+  expose :is_dismissed_suggest_pipeline, if: -> (*) { Gitlab::Experimentation.enabled?(:suggest_pipeline) } do |_merge_request|
     current_user && current_user.dismissed_callout?(feature_name: SUGGEST_PIPELINE)
   end
 

config/feature_flags/development/suggest_pipeline.yml

----
-name: suggest_pipeline
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/25547
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/212896
-group: group::expansion
-type: development
-default_enabled: false

doc/development/experiment_guide/index.md

@@ -32,38 +32,62 @@ addressed.
 
 ## How to create an A/B test
 
-- Add the experiment to the `Gitlab::Experimentation::EXPERIMENTS` hash in [`experimentation.rb`](https://gitlab.com/gitlab-org/gitlab/blob/master/lib%2Fgitlab%2Fexperimentation.rb):
-
-  ```ruby
-  EXPERIMENTS = {
-    other_experiment: {
-      #...
-    },
-    # Add your experiment here:
-    signup_flow: {
-      environment: ::Gitlab.dev_env_or_com?, # Target environment, defaults to enabled for development and GitLab.com
-      tracking_category: 'Growth::Acquisition::Experiment::SignUpFlow' # Used for providing the category when setting up tracking data
-    }
-  }.freeze
-  ```
-
-- Use the experiment in a controller:
-
-  ```ruby
-  class RegistrationController < ApplicationController
-   def show
-     # experiment_enabled?(:feature_name) is also available in views and helpers
-     if experiment_enabled?(:signup_flow)
-       # render the experiment
-     else
-       # render the original version
-     end
-   end
-  end
-  ```
-
-- Track necessary events. See the [telemetry guide](../telemetry/index.md) for details.
-- After the merge request is merged, use [`chatops`](../../ci/chatops/README.md) in the
+### Implementation
+
+1. Add the experiment to the `Gitlab::Experimentation::EXPERIMENTS` hash in [`experimentation.rb`](https://gitlab.com/gitlab-org/gitlab/blob/master/lib%2Fgitlab%2Fexperimentation.rb):
+
+   ```ruby
+   EXPERIMENTS = {
+     other_experiment: {
+       #...
+     },
+     # Add your experiment here:
+     signup_flow: {
+       environment: ::Gitlab.dev_env_or_com?, # Target environment, defaults to enabled for development and GitLab.com
+       tracking_category: 'Growth::Acquisition::Experiment::SignUpFlow' # Used for providing the category when setting up tracking data
+     }
+   }.freeze
+   ```
+
+1. Use the experiment in the code.
+
+   - Use this standard for the experiment in a controller:
+
+      ```ruby
+      class RegistrationController < ApplicationController
+       def show
+         # experiment_enabled?(:experiment_key) is also available in views and helpers
+         if experiment_enabled?(:signup_flow)
+           # render the experiment
+         else
+           # render the original version
+         end
+       end
+      end
+      ```
+
+   - Make the experiment available to the frontend in a controller:
+    
+      ```ruby
+      before_action do
+        push_frontend_experiment(:signup_flow)
+      end 
+      ```
+      
+      The above will check whether the experiment is enabled and push the result to the frontend.
+      
+      You can check the state of the feature flag in JavaScript:
+      
+      ```javascript
+      import { isExperimentEnabled } from '~/experimentation';
+      
+      if ( isExperimentEnabled('signupFlow') ) {
+        // ...
+      }
+      ```
+  
+1. Track necessary events. See the [telemetry guide](../telemetry/index.md) for details.
+1. After the merge request is merged, use [`chatops`](../../ci/chatops/README.md) in the
 [appropriate channel](../feature_flags/controls.md#communicate-the-change) to start the experiment for 10% of the users.
 The feature flag should have the name of the experiment with the `_experiment_percentage` suffix appended.
 For visibility, please also share any commands run against production in the `#s_growth` channel:
@@ -77,3 +101,19 @@ For visibility, please also share any commands run against production in the `#s
   ```shell
   /chatops run feature delete signup_flow_experiment_percentage
   ```
+
+### Tests and test helpers
+
+Use the following in Jest to test the experiment is enabled.
+
+```javascript
+import { withGonExperiment } from 'helpers/experimentation_helper';
+
+describe('given experiment is enabled', () => {
+  withGonExperiment('signupFlow');
+
+  it('should do the experimental thing', () => {
+    expect(wrapper.find('.js-some-experiment-triggered-element')).toEqual(expect.any(Element));
+  });
+});
+```

lib/gitlab/experimentation.rb

@@ -97,6 +97,13 @@ module Gitlab
         }
       end
 
+      def push_frontend_experiment(experiment_key)
+        var_name = experiment_key.to_s.camelize(:lower)
+        enabled = experiment_enabled?(experiment_key)
+
+        gon.push({ experiments: { var_name => enabled } }, true)
+      end
+
       def experiment_enabled?(experiment_key)
         return false if dnt_enabled?
 

spec/features/merge_request/user_sees_suggest_pipeline_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Merge request > User sees suggest pipeline', :js do
+  let(:merge_request) { create(:merge_request) }
+  let(:project) { merge_request.source_project }
+  let(:user) { project.creator }
+
+  before do
+    stub_application_setting(auto_devops_enabled: false)
+    stub_experiment(suggest_pipeline: true)
+    stub_experiment_for_user(suggest_pipeline: true)
+    project.add_maintainer(user)
+    sign_in(user)
+    visit project_merge_request_path(project, merge_request)
+  end
+
+  it 'shows the suggest pipeline widget and then allows dismissal correctly' do
+    expect(page).to have_content('Are you adding technical debt or code vulnerabilities?')
+
+    page.within '.mr-pipeline-suggest' do
+      find('[data-testid="close"]').click
+    end
+
+    wait_for_requests
+
+    expect(page).not_to have_content('Are you adding technical debt or code vulnerabilities?')
+
+    # Reload so we know the user callout was registered
+    visit page.current_url
+
+    expect(page).not_to have_content('Are you adding technical debt or code vulnerabilities?')
+  end
+end

spec/features/projects/blobs/user_follows_pipeline_suggest_nudge_spec.rb

@@ -10,7 +10,7 @@ RSpec.describe 'User follows pipeline suggest nudge spec when feature is enabled
 
   describe 'viewing the new blob page' do
     before do
-      stub_feature_flags(suggest_pipeline: true)
+      stub_experiment_for_user(suggest_pipeline: true)
       sign_in(user)
     end
 

spec/frontend/helpers/experimentation_helper.js

+import { merge } from 'lodash';
+
+export function withGonExperiment(experimentKey, value = true) {
+  let origGon;
+
+  beforeEach(() => {
+    origGon = window.gon;
+    window.gon = merge({}, window.gon || {}, { experiments: { [experimentKey]: value } });
+  });
+
+  afterEach(() => {
+    window.gon = origGon;
+  });
+}

spec/frontend/lib/utils/experimentation_spec.js

+import * as experimentUtils from '~/lib/utils/experimentation';
+
+const TEST_KEY = 'abc';
+
+describe('experiment Utilities', () => {
+  describe('isExperimentEnabled', () => {
+    it.each`
+      experiments              | value
+      ${{ [TEST_KEY]: true }}  | ${true}
+      ${{ [TEST_KEY]: false }} | ${false}
+      ${{ def: true }}         | ${false}
+      ${{}}                    | ${false}
+      ${null}                  | ${false}
+    `('returns correct value of $value for experiments=$experiments', ({ experiments, value }) => {
+      window.gon = { experiments };
+
+      expect(experimentUtils.isExperimentEnabled(TEST_KEY)).toEqual(value);
+    });
+  });
+});

spec/frontend/vue_mr_widget/mr_widget_options_spec.js

 import Vue from 'vue';
 import MockAdapter from 'axios-mock-adapter';
 import mountComponent from 'helpers/vue_mount_component_helper';
+import { withGonExperiment } from 'helpers/experimentation_helper';
 import axios from '~/lib/utils/axios_utils';
 import mrWidgetOptions from '~/vue_merge_request_widget/mr_widget_options.vue';
 import eventHub from '~/vue_merge_request_widget/event_hub';
@@ -812,43 +813,61 @@ describe('mrWidgetOptions', () => {
     });
   });
 
-  describe('given suggestPipeline feature flag is enabled', () => {
+  describe('suggestPipeline Experiment', () => {
     beforeEach(() => {
       mock.onAny().reply(200);
 
       // This is needed because some grandchildren Bootstrap components throw warnings
       // https://gitlab.com/gitlab-org/gitlab/issues/208458
       jest.spyOn(console, 'warn').mockImplementation();
+    });
 
-      gon.features = { suggestPipeline: true };
+    describe('given experiment is enabled', () => {
+      withGonExperiment('suggestPipeline');
 
-      createComponent();
+      beforeEach(() => {
+        createComponent();
 
-      vm.mr.hasCI = false;
-    });
+        vm.mr.hasCI = false;
+      });
 
-    it('should suggest pipelines when none exist', () => {
-      expect(findSuggestPipeline()).toEqual(expect.any(Element));
-    });
+      it('should suggest pipelines when none exist', () => {
+        expect(findSuggestPipeline()).toEqual(expect.any(Element));
+      });
 
-    it.each([
-      { isDismissedSuggestPipeline: true },
-      { mergeRequestAddCiConfigPath: null },
-      { hasCI: true },
-    ])('with %s, should not suggest pipeline', async obj => {
-      Object.assign(vm.mr, obj);
+      it.each([
+        { isDismissedSuggestPipeline: true },
+        { mergeRequestAddCiConfigPath: null },
+        { hasCI: true },
+      ])('with %s, should not suggest pipeline', async obj => {
+        Object.assign(vm.mr, obj);
 
-      await vm.$nextTick();
+        await vm.$nextTick();
 
-      expect(findSuggestPipeline()).toBeNull();
+        expect(findSuggestPipeline()).toBeNull();
+      });
+
+      it('should allow dismiss of the suggest pipeline message', async () => {
+        findSuggestPipelineButton().click();
+
+        await vm.$nextTick();
+
+        expect(findSuggestPipeline()).toBeNull();
+      });
     });
 
-    it('should allow dismiss of the suggest pipeline message', async () => {
-      findSuggestPipelineButton().click();
+    describe('given suggestPipeline experiment is not enabled', () => {
+      withGonExperiment('suggestPipeline', false);
 
-      await vm.$nextTick();
+      beforeEach(() => {
+        createComponent();
 
-      expect(findSuggestPipeline()).toBeNull();
+        vm.mr.hasCI = false;
+      });
+
+      it('should not suggest pipelines when none exist', () => {
+        expect(findSuggestPipeline()).toBeNull();
+      });
     });
   });
 });

spec/lib/gitlab/experimentation_spec.rb

@@ -69,6 +69,20 @@ RSpec.describe Gitlab::Experimentation do
       end
     end
 
+    describe '#push_frontend_experiment' do
+      it 'pushes an experiment to the frontend' do
+        gon = instance_double('gon')
+        experiments = { experiments: { 'myExperiment' => true } }
+
+        stub_experiment_for_user(my_experiment: true)
+        allow(controller).to receive(:gon).and_return(gon)
+
+        expect(gon).to receive(:push).with(experiments, true)
+
+        controller.push_frontend_experiment(:my_experiment)
+      end
+    end
+
     describe '#experiment_enabled?' do
       subject { controller.experiment_enabled?(:test_experiment) }
 

spec/serializers/merge_request_widget_entity_spec.rb

@@ -272,7 +272,7 @@ RSpec.describe MergeRequestWidgetEntity do
   describe 'user callouts' do
     context 'when suggest pipeline feature is enabled' do
       before do
-        stub_feature_flags(suggest_pipeline: true)
+        stub_experiment(suggest_pipeline: true)
       end
 
       it 'provides a valid path value for user callout path' do
@@ -308,7 +308,7 @@ RSpec.describe MergeRequestWidgetEntity do
 
     context 'when suggest pipeline feature is not enabled' do
       before do
-        stub_feature_flags(suggest_pipeline: false)
+        stub_experiment(suggest_pipeline: false)
       end
 
       it 'provides no valid value for user callout path' do