Merge branch 'remove_mr_tabs_position_feature_flag' into 'master'

Remove `mr_tabs_position` feature flag

Closes #196174

See merge request gitlab-org/gitlab!29340

app/assets/javascripts/mr_tabs_popover/components/popover.vue

-<script>
-import { GlPopover, GlDeprecatedButton, GlLink } from '@gitlab/ui';
-import Icon from '~/vue_shared/components/icon.vue';
-import axios from '~/lib/utils/axios_utils';
-
-export default {
-  components: {
-    GlPopover,
-    GlDeprecatedButton,
-    GlLink,
-    Icon,
-  },
-  props: {
-    dismissEndpoint: {
-      type: String,
-      required: true,
-    },
-    featureId: {
-      type: String,
-      required: true,
-    },
-  },
-  data() {
-    return {
-      showPopover: false,
-    };
-  },
-  mounted() {
-    setTimeout(() => {
-      this.showPopover = true;
-    }, 2000);
-  },
-  methods: {
-    onDismiss() {
-      this.showPopover = false;
-
-      axios.post(this.dismissEndpoint, {
-        feature_name: this.featureId,
-      });
-    },
-  },
-};
-</script>
-
-<template>
-  <gl-popover target="#diffs-tab" placement="bottom" :show="showPopover">
-    <p class="mb-2">
-      {{
-        __(
-          'Now you can access the merge request navigation tabs at the top, where they’re easier to find.',
-        )
-      }}
-    </p>
-    <p>
-      <gl-link href="https://gitlab.com/gitlab-org/gitlab/issues/36125" target="_blank">
-        {{ __('More information and share feedback') }}
-        <icon name="external-link" :size="10" />
-      </gl-link>
-    </p>
-    <gl-deprecated-button
-      variant="primary"
-      size="sm"
-      data-qa-selector="dismiss_popover_button"
-      @click="onDismiss"
-    >
-      {{ __('Got it') }}
-    </gl-deprecated-button>
-  </gl-popover>
-</template>

app/assets/javascripts/mr_tabs_popover/index.js

-import Vue from 'vue';
-import Popover from './components/popover.vue';
-
-export default el =>
-  new Vue({
-    el,
-    render(createElement) {
-      return createElement(Popover, {
-        props: { dismissEndpoint: el.dataset.dismissEndpoint, featureId: el.dataset.featureId },
-      });
-    },
-  });

app/assets/javascripts/pages/projects/merge_requests/init_merge_request_show.js

@@ -6,7 +6,6 @@ import howToMerge from '~/how_to_merge';
 import initPipelines from '~/commit/pipelines/pipelines_bundle';
 import initVueIssuableSidebarApp from '~/issuable_sidebar/sidebar_bundle';
 import initSourcegraph from '~/sourcegraph';
-import initPopover from '~/mr_tabs_popover';
 
 export default function() {
   new ZenMode(); // eslint-disable-line no-new
@@ -20,10 +19,4 @@ export default function() {
   handleLocationHash();
   howToMerge();
   initSourcegraph();
-
-  const tabHighlightEl = document.querySelector('.js-tabs-feature-highlight');
-
-  if (tabHighlightEl) {
-    initPopover(tabHighlightEl);
-  }
 }

app/helpers/merge_requests_helper.rb

@@ -170,12 +170,6 @@ module MergeRequestsHelper
       current_user.fork_of(project)
     end
   end
-
-  def mr_tabs_position_enabled?
-    strong_memoize(:mr_tabs_position_enabled) do
-      Feature.enabled?(:mr_tabs_position, @project, default_enabled: true)
-    end
-  end
 end
 
 MergeRequestsHelper.prepend_if_ee('EE::MergeRequestsHelper')

app/helpers/user_callouts_helper.rb

@@ -37,10 +37,6 @@ module UserCalloutsHelper
     !user_dismissed?(SUGGEST_POPOVER_DISMISSED)
   end
 
-  def show_tabs_feature_highlight?
-    current_user && !user_dismissed?(TABS_POSITION_HIGHLIGHT) && !Rails.env.test?
-  end
-
   def show_webhooks_moved_alert?
     !user_dismissed?(WEBHOOKS_MOVED)
   end

app/views/layouts/nav/_breadcrumbs.html.haml

@@ -2,7 +2,7 @@
 - hide_top_links = @hide_top_links || false
 
 %nav.breadcrumbs{ role: "navigation", class: [container, @content_class] }
-  .breadcrumbs-container{ class: ("border-bottom-0" if @no_breadcrumb_border && mr_tabs_position_enabled?) }
+  .breadcrumbs-container{ class: ("border-bottom-0" if @no_breadcrumb_border) }
     - if defined?(@left_sidebar)
       = button_tag class: 'toggle-mobile-nav', type: 'button' do
         %span.sr-only= _("Open sidebar")

app/views/projects/merge_requests/_awards_block.html.haml

 .content-block.content-block-small.emoji-list-container.js-noteable-awards
   = render 'award_emoji/awards_block', awardable: @merge_request, inline: true do
-    - if mr_tabs_position_enabled?
-      .ml-auto.mt-auto.mb-auto
-        #js-vue-sort-issue-discussions
-        = render "projects/merge_requests/discussion_filter"
+    .ml-auto.mt-auto.mb-auto
+      #js-vue-sort-issue-discussions
+      = render "projects/merge_requests/discussion_filter"

app/views/projects/merge_requests/_mr_box.html.haml

-.detail-page-description{ class: ("py-2" if mr_tabs_position_enabled?) }
-  %h2.title.qa-title{ class: ("mb-0" if mr_tabs_position_enabled?) }
+.detail-page-description.py-2
+  %h2.title.qa-title.mb-0
     = markdown_field(@merge_request, :title)
-
-  - unless mr_tabs_position_enabled?
-    = render "projects/merge_requests/description"

app/views/projects/merge_requests/_mr_title.html.haml

@@ -7,7 +7,7 @@
   .alert.alert-danger
     The source project of this merge request has been removed.
 
-.detail-page-header{ class: ("border-bottom-0 pt-0 pb-0" if mr_tabs_position_enabled?) }
+.detail-page-header.border-bottom-0.pt-0.pb-0
   .detail-page-header-body
     .issuable-status-box.status-box{ class: status_box_class(@merge_request) }
       = sprite_icon(state_icon_name, css_class: 'd-block d-sm-none')

app/views/projects/merge_requests/show.html.haml

@@ -17,20 +17,12 @@
 
   .merge-request-details.issuable-details{ data: { id: @merge_request.project.id } }
     = render "projects/merge_requests/mr_box"
-
-    - unless mr_tabs_position_enabled?
-      = render "projects/merge_requests/widget"
-      = render "projects/merge_requests/awards_block"
-
     .merge-request-tabs-holder{ class: ("js-tabs-affix" unless ENV['RAILS_ENV'] == 'test') }
       .merge-request-tabs-container
         %ul.merge-request-tabs.nav-tabs.nav.nav-links
           = render "projects/merge_requests/tabs/tab", class: "notes-tab", qa_selector: "notes_tab" do
             = tab_link_for @merge_request, :show, force_link: @commit.present? do
-              - if mr_tabs_position_enabled?
-                = _("Overview")
-              - else
-                = _("Discussion")
+              = _("Overview")
               %span.badge.badge-pill= @merge_request.related_notes.user.count
           - if @merge_request.source_project
             = render "projects/merge_requests/tabs/tab", name: "commits", class: "commits-tab" do
@@ -46,11 +38,7 @@
             = tab_link_for @merge_request, :diffs do
               = _("Changes")
               %span.badge.badge-pill= @merge_request.diff_size
-            - if mr_tabs_position_enabled? && show_tabs_feature_highlight?
-              .js-tabs-feature-highlight{ data: { dismiss_endpoint: user_callouts_path, feature_id: UserCalloutsHelper::TABS_POSITION_HIGHLIGHT } }
         .d-flex.flex-wrap.align-items-center.justify-content-lg-end
-          - unless mr_tabs_position_enabled?
-            = render "projects/merge_requests/discussion_filter"
           #js-vue-discussion-counter
 
     .tab-content#diff-notes-app
@@ -62,12 +50,11 @@
           %section.col-md-12
             %script.js-notes-data{ type: "application/json" }= initial_notes_data(true).to_json.html_safe
             .issuable-discussion.js-vue-notes-event
-              - if mr_tabs_position_enabled?
-                - if @merge_request.description.present?
-                  .detail-page-description
-                    = render "projects/merge_requests/description"
-                = render "projects/merge_requests/widget"
-                = render "projects/merge_requests/awards_block"
+              - if @merge_request.description.present?
+                .detail-page-description
+                  = render "projects/merge_requests/description"
+              = render "projects/merge_requests/widget"
+              = render "projects/merge_requests/awards_block"
               #js-vue-mr-discussions{ data: { notes_data: notes_data(@merge_request).to_json,
                 noteable_data: serialize_issuable(@merge_request, serializer: 'noteable'),
                 noteable_type: 'MergeRequest',

changelogs/unreleased/remove_mr_tabs_position_feature_flag.yml

+---
+title: Remove mr_tabs_position feature flag
+merge_request: 29340
+author: Lee Tickett
+type: other

locale/gitlab.pot

@@ -16763,9 +16763,6 @@ msgstr ""
 msgid "Novice"
 msgstr ""
 
-msgid "Now you can access the merge request navigation tabs at the top, where they’re easier to find."
-msgstr ""
-
 msgid "Nuget metadatum must have at least license_url, project_url or icon_url set"
 msgstr ""
 

qa/qa/page/merge_request/show.rb

@@ -7,10 +7,6 @@ module QA
         include Page::Component::Note
         include Page::Component::Issuable::Sidebar
 
-        view 'app/assets/javascripts/mr_tabs_popover/components/popover.vue' do
-          element :dismiss_popover_button
-        end
-
         view 'app/assets/javascripts/vue_merge_request_widget/components/mr_widget_header.vue' do
           element :dropdown_toggle
           element :download_email_patches

spec/features/merge_request/user_sees_merge_request_pipelines_spec.rb

@@ -8,7 +8,6 @@ RSpec.describe 'Merge request > User sees pipelines triggered by merge request',
 
   let(:project) { create(:project, :public, :repository) }
   let(:user) { project.creator }
-  let(:enable_mr_tabs_position_flag) { true }
 
   let(:config) do
     {
@@ -27,7 +26,6 @@ RSpec.describe 'Merge request > User sees pipelines triggered by merge request',
   end
 
   before do
-    stub_feature_flags(mr_tabs_position: enable_mr_tabs_position_flag)
     stub_application_setting(auto_devops_enabled: false)
     stub_feature_flags(ci_merge_request_pipeline: true)
     stub_ci_pipeline_yaml_file(YAML.dump(config))
@@ -54,8 +52,6 @@ RSpec.describe 'Merge request > User sees pipelines triggered by merge request',
                                 .execute(:merge_request_event, merge_request: merge_request)
     end
 
-    let(:enable_mr_tabs_position_flag) { false }
-
     before do
       visit project_merge_request_path(project, merge_request)
 
@@ -71,23 +67,11 @@ RSpec.describe 'Merge request > User sees pipelines triggered by merge request',
       end
     end
 
-    context 'when merge request tabs feature flag is disabled' do
-      it 'sees the latest detached merge request pipeline as the head pipeline', :sidekiq_might_not_need_inline do
-        page.within('.ci-widget-content') do
-          expect(page).to have_content("##{detached_merge_request_pipeline.id}")
-        end
-      end
-    end
-
-    context 'when merge request tabs feature flag is enabled' do
-      let(:enable_mr_tabs_position_flag) { true }
+    it 'sees the latest detached merge request pipeline as the head pipeline', :sidekiq_might_not_need_inline do
+      click_link "Overview"
 
-      it 'sees the latest detached merge request pipeline as the head pipeline', :sidekiq_might_not_need_inline do
-        click_link "Overview"
-
-        page.within('.ci-widget-content') do
-          expect(page).to have_content("##{detached_merge_request_pipeline.id}")
-        end
+      page.within('.ci-widget-content') do
+        expect(page).to have_content("##{detached_merge_request_pipeline.id}")
       end
     end
 
@@ -145,6 +129,8 @@ RSpec.describe 'Merge request > User sees pipelines triggered by merge request',
       end
 
       it 'sees the latest detached merge request pipeline as the head pipeline' do
+        click_link 'Overview'
+
         page.within('.ci-widget-content') do
           expect(page).to have_content("##{detached_merge_request_pipeline_2.id}")
         end
@@ -153,6 +139,7 @@ RSpec.describe 'Merge request > User sees pipelines triggered by merge request',
 
     context 'when a user merges a merge request in the parent project', :sidekiq_might_not_need_inline do
       before do
+        click_link 'Overview'
         click_button 'Merge when pipeline succeeds'
 
         wait_for_requests
@@ -180,6 +167,7 @@ RSpec.describe 'Merge request > User sees pipelines triggered by merge request',
 
       context 'when branch pipeline succeeds' do
         before do
+          click_link 'Overview'
           push_pipeline.succeed!
 
           wait_for_requests
@@ -215,6 +203,8 @@ RSpec.describe 'Merge request > User sees pipelines triggered by merge request',
       end
 
       it 'sees the latest branch pipeline as the head pipeline', :sidekiq_might_not_need_inline do
+        click_link 'Overview'
+
         page.within('.ci-widget-content') do
           expect(page).to have_content("##{push_pipeline.id}")
         end
@@ -261,23 +251,11 @@ RSpec.describe 'Merge request > User sees pipelines triggered by merge request',
       end
     end
 
-    context 'when merge request tabs feature flag is enabled' do
-      it 'sees the latest detached merge request pipeline as the head pipeline' do
-        click_link "Overview"
+    it 'sees the latest detached merge request pipeline as the head pipeline' do
+      click_link "Overview"
 
-        page.within('.ci-widget-content') do
-          expect(page).to have_content("##{detached_merge_request_pipeline.id}")
-        end
-      end
-    end
-
-    context 'when merge request tabs feature flag is disabled' do
-      let(:enable_mr_tabs_position_flag) { false }
-
-      it 'sees the latest detached merge request pipeline as the head pipeline' do
-        page.within('.ci-widget-content') do
-          expect(page).to have_content("##{detached_merge_request_pipeline.id}")
-        end
+      page.within('.ci-widget-content') do
+        expect(page).to have_content("##{detached_merge_request_pipeline.id}")
       end
     end