Commit 5f880446 authored by Doug Stull's avatar Doug Stull Committed by Paul Slaughter

Resolve suggest_pipeline flag naming issues

- we were referring to it as suggest_pipeline in some
  places and suggest_pipeline_experiment_percentage in others.

https://gitlab.com/gitlab-org/gitlab/-/merge_requests/42985
parent 19a0c69e
export function isExperimentEnabled(experimentKey) {
return Boolean(window.gon?.experiments?.[experimentKey]);
}
...@@ -6,6 +6,7 @@ import GpgBadges from '~/gpg_badges'; ...@@ -6,6 +6,7 @@ import GpgBadges from '~/gpg_badges';
import '~/sourcegraph/load'; import '~/sourcegraph/load';
import PipelineTourSuccessModal from '~/blob/pipeline_tour_success_modal.vue'; import PipelineTourSuccessModal from '~/blob/pipeline_tour_success_modal.vue';
import { parseBoolean } from '~/lib/utils/common_utils'; import { parseBoolean } from '~/lib/utils/common_utils';
import { isExperimentEnabled } from '~/lib/utils/experimentation';
const createGitlabCiYmlVisualization = (containerId = '#js-blob-toggle-graph-preview') => { const createGitlabCiYmlVisualization = (containerId = '#js-blob-toggle-graph-preview') => {
const el = document.querySelector(containerId); const el = document.querySelector(containerId);
...@@ -70,7 +71,7 @@ document.addEventListener('DOMContentLoaded', () => { ...@@ -70,7 +71,7 @@ document.addEventListener('DOMContentLoaded', () => {
); );
} }
if (gon.features?.suggestPipeline) { if (isExperimentEnabled('suggestPipeline')) {
const successPipelineEl = document.querySelector('.js-success-pipeline-modal'); const successPipelineEl = document.querySelector('.js-success-pipeline-modal');
if (successPipelineEl) { if (successPipelineEl) {
......
...@@ -45,6 +45,7 @@ import GroupedTestReportsApp from '../reports/components/grouped_test_reports_ap ...@@ -45,6 +45,7 @@ import GroupedTestReportsApp from '../reports/components/grouped_test_reports_ap
import { setFaviconOverlay } from '../lib/utils/common_utils'; import { setFaviconOverlay } from '../lib/utils/common_utils';
import GroupedAccessibilityReportsApp from '../reports/accessibility_report/grouped_accessibility_reports_app.vue'; import GroupedAccessibilityReportsApp from '../reports/accessibility_report/grouped_accessibility_reports_app.vue';
import getStateQuery from './queries/get_state.query.graphql'; import getStateQuery from './queries/get_state.query.graphql';
import { isExperimentEnabled } from '~/lib/utils/experimentation';
export default { export default {
el: '#js-vue-mr-widget', el: '#js-vue-mr-widget',
...@@ -148,7 +149,7 @@ export default { ...@@ -148,7 +149,7 @@ export default {
}, },
shouldSuggestPipelines() { shouldSuggestPipelines() {
return ( return (
gon.features?.suggestPipeline && isExperimentEnabled('suggestPipeline') &&
!this.mr.hasCI && !this.mr.hasCI &&
this.mr.mergeRequestAddCiConfigPath && this.mr.mergeRequestAddCiConfigPath &&
!this.mr.isDismissedSuggestPipeline !this.mr.isDismissedSuggestPipeline
......
...@@ -33,7 +33,7 @@ class Projects::BlobController < Projects::ApplicationController ...@@ -33,7 +33,7 @@ class Projects::BlobController < Projects::ApplicationController
before_action :set_last_commit_sha, only: [:edit, :update] before_action :set_last_commit_sha, only: [:edit, :update]
before_action only: :show do 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) push_frontend_feature_flag(:gitlab_ci_yml_preview, @project, default_enabled: false)
end end
......
...@@ -27,7 +27,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -27,7 +27,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
before_action :authenticate_user!, only: [:assign_related_issues] before_action :authenticate_user!, only: [:assign_related_issues]
before_action :check_user_can_push_to_source_branch!, only: [:rebase] before_action :check_user_can_push_to_source_branch!, only: [:rebase]
before_action only: [:show] do 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(:widget_visibility_polling, @project, default_enabled: true)
push_frontend_feature_flag(:merge_ref_head_comments, @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) push_frontend_feature_flag(:mr_commit_neighbor_nav, @project, default_enabled: true)
......
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
module SuggestPipelineHelper module SuggestPipelineHelper
def should_suggest_gitlab_ci_yml? def should_suggest_gitlab_ci_yml?
Feature.enabled?(:suggest_pipeline) && experiment_enabled?(:suggest_pipeline) &&
current_user && current_user &&
params[:suggest_gitlab_ci_yml] == 'true' params[:suggest_gitlab_ci_yml] == 'true'
end end
......
...@@ -67,15 +67,15 @@ class MergeRequestWidgetEntity < Grape::Entity ...@@ -67,15 +67,15 @@ class MergeRequestWidgetEntity < Grape::Entity
) )
end 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 user_callouts_path
end 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 SUGGEST_PIPELINE
end 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) current_user && current_user.dismissed_callout?(feature_name: SUGGEST_PIPELINE)
end end
......
---
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
...@@ -32,38 +32,62 @@ addressed. ...@@ -32,38 +32,62 @@ addressed.
## How to create an A/B test ## 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): ### Implementation
```ruby 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):
EXPERIMENTS = {
other_experiment: { ```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 # Add your experiment here:
tracking_category: 'Growth::Acquisition::Experiment::SignUpFlow' # Used for providing the category when setting up tracking data signup_flow: {
} environment: ::Gitlab.dev_env_or_com?, # Target environment, defaults to enabled for development and GitLab.com
}.freeze tracking_category: 'Growth::Acquisition::Experiment::SignUpFlow' # Used for providing the category when setting up tracking data
``` }
}.freeze
- Use the experiment in a controller: ```
```ruby 1. Use the experiment in the code.
class RegistrationController < ApplicationController
def show - Use this standard for the experiment in a controller:
# experiment_enabled?(:feature_name) is also available in views and helpers
if experiment_enabled?(:signup_flow) ```ruby
# render the experiment class RegistrationController < ApplicationController
else def show
# render the original version # experiment_enabled?(:experiment_key) is also available in views and helpers
end if experiment_enabled?(:signup_flow)
end # render the experiment
end else
``` # render the original version
end
- Track necessary events. See the [telemetry guide](../telemetry/index.md) for details. end
- After the merge request is merged, use [`chatops`](../../ci/chatops/README.md) in the 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. [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. 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: 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 ...@@ -77,3 +101,19 @@ For visibility, please also share any commands run against production in the `#s
```shell ```shell
/chatops run feature delete signup_flow_experiment_percentage /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));
});
});
```
...@@ -97,6 +97,13 @@ module Gitlab ...@@ -97,6 +97,13 @@ module Gitlab
} }
end 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) def experiment_enabled?(experiment_key)
return false if dnt_enabled? return false if dnt_enabled?
......
# 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
...@@ -10,7 +10,7 @@ RSpec.describe 'User follows pipeline suggest nudge spec when feature is enabled ...@@ -10,7 +10,7 @@ RSpec.describe 'User follows pipeline suggest nudge spec when feature is enabled
describe 'viewing the new blob page' do describe 'viewing the new blob page' do
before do before do
stub_feature_flags(suggest_pipeline: true) stub_experiment_for_user(suggest_pipeline: true)
sign_in(user) sign_in(user)
end end
......
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;
});
}
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);
});
});
});
import Vue from 'vue'; import Vue from 'vue';
import MockAdapter from 'axios-mock-adapter'; import MockAdapter from 'axios-mock-adapter';
import mountComponent from 'helpers/vue_mount_component_helper'; import mountComponent from 'helpers/vue_mount_component_helper';
import { withGonExperiment } from 'helpers/experimentation_helper';
import axios from '~/lib/utils/axios_utils'; import axios from '~/lib/utils/axios_utils';
import mrWidgetOptions from '~/vue_merge_request_widget/mr_widget_options.vue'; import mrWidgetOptions from '~/vue_merge_request_widget/mr_widget_options.vue';
import eventHub from '~/vue_merge_request_widget/event_hub'; import eventHub from '~/vue_merge_request_widget/event_hub';
...@@ -812,43 +813,61 @@ describe('mrWidgetOptions', () => { ...@@ -812,43 +813,61 @@ describe('mrWidgetOptions', () => {
}); });
}); });
describe('given suggestPipeline feature flag is enabled', () => { describe('suggestPipeline Experiment', () => {
beforeEach(() => { beforeEach(() => {
mock.onAny().reply(200); mock.onAny().reply(200);
// This is needed because some grandchildren Bootstrap components throw warnings // This is needed because some grandchildren Bootstrap components throw warnings
// https://gitlab.com/gitlab-org/gitlab/issues/208458 // https://gitlab.com/gitlab-org/gitlab/issues/208458
jest.spyOn(console, 'warn').mockImplementation(); 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', () => { it('should suggest pipelines when none exist', () => {
expect(findSuggestPipeline()).toEqual(expect.any(Element)); expect(findSuggestPipeline()).toEqual(expect.any(Element));
}); });
it.each([ it.each([
{ isDismissedSuggestPipeline: true }, { isDismissedSuggestPipeline: true },
{ mergeRequestAddCiConfigPath: null }, { mergeRequestAddCiConfigPath: null },
{ hasCI: true }, { hasCI: true },
])('with %s, should not suggest pipeline', async obj => { ])('with %s, should not suggest pipeline', async obj => {
Object.assign(vm.mr, 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 () => { describe('given suggestPipeline experiment is not enabled', () => {
findSuggestPipelineButton().click(); 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();
});
}); });
}); });
}); });
...@@ -69,6 +69,20 @@ RSpec.describe Gitlab::Experimentation do ...@@ -69,6 +69,20 @@ RSpec.describe Gitlab::Experimentation do
end end
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 describe '#experiment_enabled?' do
subject { controller.experiment_enabled?(:test_experiment) } subject { controller.experiment_enabled?(:test_experiment) }
......
...@@ -272,7 +272,7 @@ RSpec.describe MergeRequestWidgetEntity do ...@@ -272,7 +272,7 @@ RSpec.describe MergeRequestWidgetEntity do
describe 'user callouts' do describe 'user callouts' do
context 'when suggest pipeline feature is enabled' do context 'when suggest pipeline feature is enabled' do
before do before do
stub_feature_flags(suggest_pipeline: true) stub_experiment(suggest_pipeline: true)
end end
it 'provides a valid path value for user callout path' do it 'provides a valid path value for user callout path' do
...@@ -308,7 +308,7 @@ RSpec.describe MergeRequestWidgetEntity do ...@@ -308,7 +308,7 @@ RSpec.describe MergeRequestWidgetEntity do
context 'when suggest pipeline feature is not enabled' do context 'when suggest pipeline feature is not enabled' do
before do before do
stub_feature_flags(suggest_pipeline: false) stub_experiment(suggest_pipeline: false)
end end
it 'provides no valid value for user callout path' do it 'provides no valid value for user callout path' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment