Merge branch '352177-cleanup-billing-nav-experiment' into 'master'

Remove the billing in side nav experiment

See merge request gitlab-org/gitlab!80306

ee/app/controllers/groups/billings_controller.rb

@@ -21,9 +21,7 @@ class Groups::BillingsController < Groups::ApplicationController
       .new(plan: current_plan, namespace_id: relevant_group.id)
       .execute
 
-    if @plans_data
-      track_from_side_nav(relevant_group)
-    else
+    unless @plans_data
       render 'shared/billings/customers_dot_unavailable'
     end
   end
@@ -50,10 +48,4 @@ class Groups::BillingsController < Groups::ApplicationController
     gitlab_subscription.refresh_seat_attributes!
     gitlab_subscription.save
   end
-
-  def track_from_side_nav(relevant_group)
-    return unless helpers.accessed_billing_from_side_nav?
-
-    experiment(:billing_in_side_nav, actor: current_user, namespace: relevant_group, sticky_to: current_user).track(:view, label: 'view_billing')
-  end
 end

ee/app/helpers/billing_plans_helper.rb

@@ -152,38 +152,24 @@ module BillingPlansHelper
   end
 
   def billing_upgrade_button_data(plan)
-    data = {
+    {
       track_action: 'click_button',
       track_label: 'upgrade',
       track_property: plan.code,
       qa_selector: "upgrade_to_#{plan.code}"
     }
-
-    add_billing_in_side_nav_attribute(data)
   end
 
   def start_free_trial_data
-    data = {
+    {
       track_action: 'click_button',
       track_label: 'start_trial',
       qa_selector: 'start_your_free_trial'
     }
-
-    add_billing_in_side_nav_attribute(data)
-  end
-
-  def accessed_billing_from_side_nav?
-    params[:from] == 'side_nav'
   end
 
   private
 
-  def add_billing_in_side_nav_attribute(data)
-    return data unless accessed_billing_from_side_nav?
-
-    data.merge!(track_experiment: :billing_in_side_nav)
-  end
-
   def add_seats_url(group)
     return unless group
 

ee/config/feature_flags/experiment/billing_in_side_nav.yml

----
-name: billing_in_side_nav
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/74934
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/344598
-milestone: '14.6'
-type: experiment
-group: group::conversion
-default_enabled: false

ee/lib/ee/sidebars/groups/menus/settings_menu.rb

@@ -7,7 +7,6 @@ module EE
         module SettingsMenu
           extend ::Gitlab::Utils::Override
           include ::Gitlab::Utils::StrongMemoize
-          include ::Gitlab::Experiment::Dsl
 
           override :configure_menu_items
           def configure_menu_items
@@ -116,22 +115,10 @@ module EE
               return ::Sidebars::NilMenuItem.new(item_id: :billing)
             end
 
-            local_active_routes = { path: 'billings#index' }
-
-            experiment(:billing_in_side_nav, actor: context.current_user, namespace: context.group.root_ancestor, sticky_to: context.current_user) do |e|
-              e.control {}
-              e.candidate do
-                local_active_routes = {
-                  page: group_billings_path(context.group),
-                  exclude_page: group_billings_path(context.group, from: :side_nav)
-                }
-              end
-            end
-
             ::Sidebars::MenuItem.new(
               title: _('Billing'),
               link: group_billings_path(context.group),
-              active_routes: local_active_routes,
+              active_routes: { path: 'billings#index' },
               item_id: :billing
             )
           end

ee/lib/ee/sidebars/groups/panel.rb

@@ -17,18 +17,6 @@ module EE
           insert_menu_after(::Sidebars::Groups::Menus::PackagesRegistriesMenu, ::Sidebars::Groups::Menus::AnalyticsMenu.new(context))
           insert_menu_after(::Sidebars::Groups::Menus::AnalyticsMenu, ::Sidebars::Groups::Menus::WikiMenu.new(context))
           insert_menu_after(::Sidebars::Groups::Menus::SettingsMenu, ::Sidebars::Groups::Menus::AdministrationMenu.new(context))
-          add_billing_sidebar_menu
-        end
-
-        private
-
-        def add_billing_sidebar_menu
-          experiment(:billing_in_side_nav, actor: context.current_user, namespace: context.group.root_ancestor, sticky_to: context.current_user) do |e|
-            e.control {}
-            e.candidate do
-              insert_menu_after(::Sidebars::Groups::Menus::AdministrationMenu, ::Sidebars::Groups::Menus::BillingMenu.new(context))
-            end
-          end
         end
       end
     end

ee/lib/ee/sidebars/projects/panel.rb

@@ -16,19 +16,6 @@ module EE
           if ::Sidebars::Projects::Menus::IssuesMenu.new(context).show_jira_menu_items?
             remove_menu(::Sidebars::Projects::Menus::ExternalIssueTrackerMenu)
           end
-
-          add_billing_sidebar_menu
-        end
-
-        private
-
-        def add_billing_sidebar_menu
-          experiment(:billing_in_side_nav, actor: context.current_user, namespace: context.project.namespace.root_ancestor, sticky_to: context.current_user) do |e|
-            e.control {}
-            e.candidate do
-              insert_menu_after(::Sidebars::Projects::Menus::SettingsMenu, ::Sidebars::Projects::Menus::BillingMenu.new(context))
-            end
-          end
         end
       end
     end

ee/lib/sidebars/groups/menus/administration_menu.rb

@@ -5,7 +5,6 @@ module Sidebars
     module Menus
       class AdministrationMenu < ::Sidebars::Menu
         include Gitlab::Utils::StrongMemoize
-        include ::Gitlab::Experiment::Dsl
 
         override :configure_menu_items
         def configure_menu_items
@@ -71,22 +70,10 @@ module Sidebars
             return ::Sidebars::NilMenuItem.new(item_id: :billing)
           end
 
-          local_active_routes = { path: 'billings#index' }
-
-          experiment(:billing_in_side_nav, actor: context.current_user, namespace: context.group.root_ancestor, sticky_to: context.current_user) do |e|
-            e.control {}
-            e.candidate do
-              local_active_routes = {
-                page: group_billings_path(context.group),
-                exclude_page: group_billings_path(context.group, from: :side_nav)
-              }
-            end
-          end
-
           ::Sidebars::MenuItem.new(
             title: _('Billing'),
             link: group_billings_path(context.group),
-            active_routes: local_active_routes,
+            active_routes: { path: 'billings#index' },
             item_id: :billing
           )
         end

ee/lib/sidebars/groups/menus/billing_menu.rb

-# frozen_string_literal: true
-
-module Sidebars
-  module Groups
-    module Menus
-      class BillingMenu < ::Sidebars::Menu
-        override :link
-        def link
-          group_billings_path(root_group, from: :side_nav)
-        end
-
-        override :title
-        def title
-          _('Billing')
-        end
-
-        override :sprite_icon
-        def sprite_icon
-          'credit-card'
-        end
-
-        override :render?
-        def render?
-          ::Gitlab::CurrentSettings.should_check_namespace_plan? &&
-            can?(context.current_user, :admin_namespace, root_group) &&
-            !root_group.user_namespace?
-        end
-
-        override :extra_container_html_options
-        def extra_container_html_options
-          {
-            class: 'shortcuts-billings'
-          }
-        end
-
-        override :extra_nav_link_html_options
-        def extra_nav_link_html_options
-          {
-            data: {
-              track_action: :render,
-              track_experiment: :billing_in_side_nav
-            }
-          }
-        end
-
-        override :active_routes
-        def active_routes
-          { page: group_billings_path(root_group, from: :side_nav) }
-        end
-
-        private
-
-        def root_group
-          context.group.root_ancestor
-        end
-      end
-    end
-  end
-end

ee/lib/sidebars/projects/menus/billing_menu.rb

-# frozen_string_literal: true
-
-module Sidebars
-  module Projects
-    module Menus
-      class BillingMenu < ::Sidebars::Groups::Menus::BillingMenu
-        private
-
-        def root_group
-          context.project.namespace.root_ancestor
-        end
-      end
-    end
-  end
-end

ee/spec/controllers/groups/billings_controller_spec.rb

@@ -63,49 +63,6 @@ RSpec.describe Groups::BillingsController, :saas do
           expect(response).to render_template('shared/billings/customers_dot_unavailable')
         end
       end
-
-      context 'when from billing link in side nav', :aggregate_failures, :experiment do
-        context 'when user comes from side nav billing link' do
-          before do
-            stub_experiments(billing_in_side_nav: :candidate)
-          end
-
-          it 'assigns the candidate experience and tracks the event' do
-            expect(experiment(:billing_in_side_nav)).to track(:view, label: 'view_billing')
-                                                                .for(:candidate)
-                                                                .with_context(actor: user, namespace: group)
-                                                                .on_next_instance
-
-            get :index, params: { group_id: group, from: :side_nav }
-
-            expect(response).not_to render_template('shared/billings/customers_dot_unavailable')
-          end
-
-          context 'when CustomersDot is unavailable' do
-            before do
-              allow_next_instance_of(GitlabSubscriptions::FetchSubscriptionPlansService) do |instance|
-                allow(instance).to receive(:execute).and_return(nil)
-              end
-            end
-
-            it 'renders a different partial' do
-              expect(experiment(:billing_in_side_nav)).not_to track(:view, label: 'view_billing')
-
-              get :index, params: { group_id: group, from: :side_nav }
-
-              expect(response).to render_template('shared/billings/customers_dot_unavailable')
-            end
-          end
-        end
-
-        context 'when user does not come from side nav link' do
-          it 'does not track the event' do
-            expect(experiment(:billing_in_side_nav)).not_to track(:view, label: 'view_billing')
-
-            get_index
-          end
-        end
-      end
     end
 
     context 'unauthorized' do

ee/spec/helpers/billing_plans_helper_spec.rb

@@ -572,15 +572,7 @@ RSpec.describe BillingPlansHelper, :saas do
       }
     end
 
-    it 'has experiment attribute' do
-      allow(helper).to receive(:params).and_return({ from: 'side_nav' })
-
-      expect(helper.billing_upgrade_button_data(plan)).to eq data.merge(track_experiment: :billing_in_side_nav)
-    end
-
-    it 'does not have experiment attribute' do
-      allow(helper).to receive(:params).and_return({})
-
+    it 'has expected data' do
       expect(helper.billing_upgrade_button_data(plan)).to eq data
     end
   end
@@ -594,30 +586,8 @@ RSpec.describe BillingPlansHelper, :saas do
       }
     end
 
-    it 'has experiment attribute' do
-      allow(helper).to receive(:params).and_return({ from: 'side_nav' })
-
-      expect(helper.start_free_trial_data).to eq data.merge(track_experiment: :billing_in_side_nav)
-    end
-
-    it 'does not have experiment attribute' do
-      allow(helper).to receive(:params).and_return({})
-
+    it 'has expected data' do
       expect(helper.start_free_trial_data).to eq data
     end
   end
-
-  describe '#accessed_billing_from_side_nav?' do
-    it 'comes from billing side nav link click' do
-      allow(helper).to receive(:params).and_return({ from: 'side_nav' })
-
-      expect(helper.accessed_billing_from_side_nav?).to eq(true)
-    end
-
-    it 'does not come from side nav link click' do
-      allow(helper).to receive(:params).and_return({})
-
-      expect(helper.accessed_billing_from_side_nav?).to eq(false)
-    end
-  end
 end

ee/spec/lib/ee/sidebars/groups/menus/settings_menu_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe Sidebars::Groups::Menus::SettingsMenu do
   let(:menu) { described_class.new(context) }
 
   describe 'Menu Items' do
-    subject(:menu_item) { menu.renderable_items.find { |e| e.item_id == item_id} }
+    subject { menu.renderable_items.find { |e| e.item_id == item_id} }
 
     describe 'LDAP sync menu' do
       let(:item_id) { :ldap_sync }
@@ -182,34 +182,6 @@ RSpec.describe Sidebars::Groups::Menus::SettingsMenu do
 
         specify { is_expected.to be_nil }
       end
-
-      context 'with billing_in_side_nav experiment', :experiment do
-        include Rails.application.routes.url_helpers
-
-        let(:settings_path) { group_billings_path(context.group) }
-
-        context 'with control experience' do
-          before do
-            stub_experiments(billing_in_side_nav: :control)
-          end
-
-          it 'does not modify the `active_routes` attribute' do
-            expect(menu_item.active_routes).to eq(path: 'billings#index')
-          end
-        end
-
-        context 'with candidate experience' do
-          before do
-            stub_experiments(billing_in_side_nav: :candidate)
-          end
-
-          it 'modifies the `active_routes` attribute' do
-            exclude_page = group_billings_path(context.group, from: :side_nav)
-
-            expect(menu_item.active_routes).to eq(page: settings_path, exclude_page: exclude_page)
-          end
-        end
-      end
     end
   end
 end

ee/spec/lib/ee/sidebars/groups/panel_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Sidebars::Groups::Panel do
-  let(:group) { build(:group, id: non_existing_record_id) }
-  let(:context) { Sidebars::Groups::Context.new(current_user: nil, container: group) }
-
-  subject(:panel) { described_class.new(context) }
-
-  describe 'BillingMenu' do
-    context 'with candidate experience' do
-      before do
-        stub_experiments(billing_in_side_nav: :candidate)
-      end
-
-      it 'contains the billing menu' do
-        expect(contains_billing_menu?).to be(true)
-      end
-    end
-
-    context 'with control experience' do
-      before do
-        stub_experiments(billing_in_side_nav: :control)
-      end
-
-      it 'does not contain the billing menu' do
-        expect(contains_billing_menu?).to be(false)
-      end
-    end
-
-    def contains_billing_menu?
-      contains_menu?(Sidebars::Groups::Menus::BillingMenu)
-    end
-  end
-
-  def contains_menu?(menu)
-    panel.instance_variable_get(:@menus).any? { |i| i.is_a?(menu) }
-  end
-end

ee/spec/lib/ee/sidebars/projects/panel_spec.rb

@@ -37,32 +37,6 @@ RSpec.describe Sidebars::Projects::Panel do
     end
   end
 
-  describe 'BillingMenu' do
-    context 'with candidate experience' do
-      before do
-        stub_experiments(billing_in_side_nav: :candidate)
-      end
-
-      it 'assigns the candidate experience and tracks the event' do
-        expect(contains_billing_menu?).to be(true)
-      end
-    end
-
-    context 'with control experience' do
-      before do
-        stub_experiments(billing_in_side_nav: :control)
-      end
-
-      it 'assigns the candidate experience and tracks the event' do
-        expect(contains_billing_menu?).to be(false)
-      end
-    end
-
-    def contains_billing_menu?
-      contains_menu?(Sidebars::Projects::Menus::BillingMenu)
-    end
-  end
-
   def contains_menu?(menu)
     panel.instance_variable_get(:@menus).any? { |i| i.is_a?(menu) }
   end

ee/spec/lib/sidebars/groups/menus/administration_menu_spec.rb

@@ -50,7 +50,7 @@ RSpec.describe Sidebars::Groups::Menus::AdministrationMenu do
   end
 
   describe 'Menu items' do
-    subject(:menu_item) { menu.renderable_items.find { |e| e.item_id == item_id } }
+    subject { menu.renderable_items.find { |e| e.item_id == item_id } }
 
     describe 'SAML SSO menu' do
       let(:item_id) { :saml_sso }
@@ -110,34 +110,6 @@ RSpec.describe Sidebars::Groups::Menus::AdministrationMenu do
 
         specify { is_expected.to be_nil }
       end
-
-      context 'with billing_in_side_nav experiment', :experiment do
-        include Rails.application.routes.url_helpers
-
-        let(:settings_path) { group_billings_path(context.group) }
-
-        context 'with control experience' do
-          before do
-            stub_experiments(billing_in_side_nav: :control)
-          end
-
-          it 'does not modify the `active_routes` attribute' do
-            expect(menu_item.active_routes).to eq(path: 'billings#index')
-          end
-        end
-
-        context 'with candidate experience' do
-          before do
-            stub_experiments(billing_in_side_nav: :candidate)
-          end
-
-          it 'modifies the `active_routes` attribute' do
-            exclude_page = group_billings_path(context.group, from: :side_nav)
-
-            expect(menu_item.active_routes).to eq(page: settings_path, exclude_page: exclude_page)
-          end
-        end
-      end
     end
   end
 end

ee/spec/lib/sidebars/groups/menus/billing_menu_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Sidebars::Groups::Menus::BillingMenu do
-  it_behaves_like 'billing menu items' do
-    let(:context) { Sidebars::Groups::Context.new(current_user: user, container: group) }
-  end
-end

ee/spec/lib/sidebars/projects/menus/billing_menu_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Sidebars::Projects::Menus::BillingMenu do
-  it_behaves_like 'billing menu items' do
-    let(:context) do
-      container = instance_double(Project, namespace: group)
-      Sidebars::Projects::Context.new(current_user: user, container: container)
-    end
-  end
-end

ee/spec/support/shared_examples/lib/sidebars/billing_menu_shared_examples.rb

-# frozen_string_literal: true
-
-RSpec.shared_examples 'billing menu items' do
-  let(:user) { instance_double(User) }
-  let(:group) { instance_double(Group) }
-  let(:namespace) { instance_double(Namespace) }
-
-  before do
-    allow(billing_menu).to receive(:can?).and_call_original
-    allow(group).to receive(:root_ancestor).and_return(namespace)
-  end
-
-  subject(:billing_menu) { described_class.new(context) }
-
-  context 'when the group can be administered' do
-    include Rails.application.routes.url_helpers
-
-    before do
-      stub_application_setting(check_namespace_plan: true)
-      allow(billing_menu).to receive(:can?).with(user, :admin_namespace, namespace).and_return(true)
-    end
-
-    describe '#title' do
-      it 'displays the correct Billing menu text for the link in the side nav' do
-        expect(billing_menu.title).to eq('Billing')
-      end
-    end
-
-    describe '#link' do
-      it 'displays the correct Billing menu text for the link in the side nav' do
-        page = group_billings_path(namespace, from: :side_nav)
-
-        expect(billing_menu.link).to eq page
-      end
-    end
-
-    describe '#active_routes' do
-      it 'uses page matching' do
-        page = group_billings_path(namespace, from: :side_nav)
-
-        expect(billing_menu.active_routes).to eq({ page: page })
-      end
-    end
-
-    describe '#extra_nav_link_html_options' do
-      it 'adds tracking attributes' do
-        data = { track_action: :render, track_experiment: :billing_in_side_nav }
-
-        expect(billing_menu.extra_nav_link_html_options).to eq({ data: data })
-      end
-    end
-
-    describe '#sprite_icon' do
-      it 'has the credit card icon' do
-        expect(billing_menu.sprite_icon).to eq 'credit-card'
-      end
-    end
-
-    describe '#extra_container_html_options' do
-      it 'has the shortcut class' do
-        expect(billing_menu.extra_container_html_options).to eq({ class: 'shortcuts-billings' })
-      end
-    end
-  end
-
-  describe '#render?' do
-    using RSpec::Parameterized::TableSyntax
-
-    before do
-      stub_application_setting(check_namespace_plan: check_namespace_plan)
-      allow(billing_menu).to receive(:can?).with(user, :admin_namespace, namespace).and_return(user_can_admin_namespace)
-      allow(namespace).to receive(:user_namespace?).and_return(user_namespace)
-    end
-
-    subject { billing_menu.render? }
-
-    where(
-      check_namespace_plan: [true, false],
-      user_can_admin_namespace: [true, false],
-      user_namespace: [true, false]
-    )
-
-    with_them do
-      it { is_expected.to eq(check_namespace_plan && user_can_admin_namespace && !user_namespace) }
-    end
-  end
-end

ee/spec/views/layouts/nav/sidebar/_group.html.haml_spec.rb

@@ -592,28 +592,4 @@ RSpec.describe 'layouts/nav/sidebar/_group' do
       expect(rendered).to have_link('Billing', href: group_billings_path(group))
     end
   end
-
-  describe 'Billing Menu' do
-    before do
-      group.add_owner(user)
-      allow(::Gitlab::CurrentSettings).to receive(:should_check_namespace_plan?).and_return(true)
-      allow(view).to receive(:current_user).and_return(user)
-    end
-
-    it 'has a link to the billing page' do
-      stub_experiments(billing_in_side_nav: :candidate)
-
-      render
-
-      expect(rendered).to have_link('Billing', href: group_billings_path(group, from: :side_nav))
-    end
-
-    it 'does not have a link to the billing page' do
-      stub_experiments(billing_in_side_nav: :control)
-
-      render
-
-      expect(rendered).not_to have_link('Billing', href: group_billings_path(group, from: :side_nav))
-    end
-  end
 end

ee/spec/views/layouts/nav/sidebar/_project.html.haml_spec.rb

@@ -373,30 +373,4 @@ RSpec.describe 'layouts/nav/sidebar/_project' do
       end
     end
   end
-
-  describe 'Billing Menu' do
-    let_it_be(:group) { create(:group).tap { |group| group.add_owner(user) } }
-
-    before do
-      allow(project).to receive(:namespace).and_return(group)
-      allow(::Gitlab::CurrentSettings).to receive(:should_check_namespace_plan?).and_return(true)
-      allow(view).to receive(:current_user).and_return(user)
-    end
-
-    it 'has a link to the billing page' do
-      stub_experiments(billing_in_side_nav: :candidate)
-
-      render
-
-      expect(rendered).to have_link('Billing', href: group_billings_path(group, from: :side_nav))
-    end
-
-    it 'does not have a link to the billing page' do
-      stub_experiments(billing_in_side_nav: :control)
-
-      render
-
-      expect(rendered).not_to have_link('Billing', href: group_billings_path(group, from: :side_nav))
-    end
-  end
 end