Merge branch 'ph/33813/moveMergeRequestDescription' into 'master'

Move merge request description into discussions tab

See merge request gitlab-org/gitlab!18940

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

+<script>
+import { GlPopover, GlButton, GlLink } from '@gitlab/ui';
+import Icon from '~/vue_shared/components/icon.vue';
+import axios from '~/lib/utils/axios_utils';
+
+export default {
+  components: {
+    GlPopover,
+    GlButton,
+    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-button variant="primary" size="sm" @click="onDismiss">
+      {{ __('Got it') }}
+    </gl-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,6 +6,7 @@ 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';
 import initWidget from '../../../vue_merge_request_widget';
 
 export default function() {
@@ -21,4 +22,10 @@ export default function() {
   howToMerge();
   initWidget();
   initSourcegraph();
+
+  const tabHighlightEl = document.querySelector('.js-tabs-feature-highlight');
+
+  if (tabHighlightEl) {
+    initPopover(tabHighlightEl);
+  }
 }

app/assets/javascripts/persistent_user_callout.js

@@ -6,8 +6,8 @@ import Flash from './flash';
 const DEFERRED_LINK_CLASS = 'deferred-link';
 
 export default class PersistentUserCallout {
-  constructor(container) {
-    const { dismissEndpoint, featureId, deferLinks } = container.dataset;
+  constructor(container, options = container.dataset) {
+    const { dismissEndpoint, featureId, deferLinks } = options;
     this.container = container;
     this.dismissEndpoint = dismissEndpoint;
     this.featureId = featureId;
@@ -53,11 +53,11 @@ export default class PersistentUserCallout {
       });
   }
 
-  static factory(container) {
+  static factory(container, options) {
     if (!container) {
       return undefined;
     }
 
-    return new PersistentUserCallout(container);
+    return new PersistentUserCallout(container, options);
   }
 }

app/helpers/merge_requests_helper.rb

 # frozen_string_literal: true
 
 module MergeRequestsHelper
+  include Gitlab::Utils::StrongMemoize
+
   def new_mr_path_from_push_event(event)
     target_project = event.project.default_merge_request_target
     project_new_merge_request_path(
@@ -168,6 +170,12 @@ 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

@@ -4,6 +4,7 @@ module UserCalloutsHelper
   GKE_CLUSTER_INTEGRATION = 'gke_cluster_integration'
   GCP_SIGNUP_OFFER = 'gcp_signup_offer'
   SUGGEST_POPOVER_DISMISSED = 'suggest_popover_dismissed'
+  TABS_POSITION_HIGHLIGHT = 'tabs_position_highlight'
 
   def show_gke_cluster_integration_callout?(project)
     can?(current_user, :create_cluster, project) &&
@@ -25,6 +26,10 @@ module UserCalloutsHelper
     !user_dismissed?(SUGGEST_POPOVER_DISMISSED)
   end
 
+  def show_tabs_feature_highlight?
+    !user_dismissed?(TABS_POSITION_HIGHLIGHT) && !Rails.env.test?
+  end
+
   private
 
   def user_dismissed?(feature_name)

app/models/user_callout_enums.rb

@@ -14,7 +14,8 @@ module UserCalloutEnums
       gke_cluster_integration: 1,
       gcp_signup_offer: 2,
       cluster_security_warning: 3,
-      suggest_popover_dismissed: 9
+      suggest_popover_dismissed: 9,
+      tabs_position_highlight: 10
     }
   end
 end

app/views/award_emoji/_awards_block.html.haml

@@ -17,3 +17,4 @@
         %span{ class: "award-control-icon award-control-icon-positive" }= sprite_icon('smiley')
         %span{ class: "award-control-icon award-control-icon-super-positive" }= sprite_icon('smile')
         = icon('spinner spin', class: "award-control-icon award-control-icon-loading")
+  = yield

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
+  .breadcrumbs-container{ class: ("border-bottom-0" if @no_breadcrumb_border && mr_tabs_position_enabled?) }
     - 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
+        = render "projects/merge_requests/discussion_filter"

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

+%div
+  - if @merge_request.description.present?
+    .description.qa-description{ class: can?(current_user, :update_merge_request, @merge_request) ? 'js-task-list-container' : '' }
+      .md
+        = markdown_field(@merge_request, :description)
+      %textarea.hidden.js-task-list-field
+        = @merge_request.description
+
+  = edited_time_ago_with_tooltip(@merge_request, placement: 'bottom')

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

+#js-vue-discussion-filter{ data: { default_filter: current_user&.notes_filter_for(@merge_request),
+  notes_filters: UserPreference.notes_filters.to_json } }

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

-.detail-page-description
-  %h2.title.qa-title
+.detail-page-description{ class: ("py-2" if mr_tabs_position_enabled?) }
+  %h2.title.qa-title{ class: ("mb-0" if mr_tabs_position_enabled?) }
     = markdown_field(@merge_request, :title)
 
-  %div
-    - if @merge_request.description.present?
-      .description.qa-description{ class: can?(current_user, :update_merge_request, @merge_request) ? 'js-task-list-container' : '' }
-        .md
-          = markdown_field(@merge_request, :description)
-        %textarea.hidden.js-task-list-field
-          = @merge_request.description
-
-  = edited_time_ago_with_tooltip(@merge_request, placement: 'bottom')
+  - unless mr_tabs_position_enabled?
+    = render "projects/merge_requests/description"

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

+- @no_breadcrumb_border = true
 - can_update_merge_request = can?(current_user, :update_merge_request, @merge_request)
 - can_reopen_merge_request = can?(current_user, :reopen_merge_request, @merge_request)
 - state_human_name, state_icon_name = state_name_with_icon(@merge_request)
@@ -6,7 +7,7 @@
   .alert.alert-danger
     The source project of this merge request has been removed.
 
-.detail-page-header
+.detail-page-header{ class: ("border-bottom-0 pt-0 pb-0" if mr_tabs_position_enabled?) }
   .detail-page-header-body
     .issuable-status-box.status-box{ class: status_box_class(@merge_request) }
       = sprite_icon(state_icon_name, size: 16, css_class: 'd-block d-sm-none')

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

+- if @merge_request.source_branch_exists?
+  = render "projects/merge_requests/how_to_merge"
+
+= javascript_tag nonce: true do
+  :plain
+    window.gl = window.gl || {};
+    window.gl.mrWidgetData = #{serialize_issuable(@merge_request, serializer: 'widget', issues_links: true)}
+
+    window.gl.mrWidgetData.squash_before_merge_help_path = '#{help_page_path("user/project/merge_requests/squash_and_merge")}';
+    window.gl.mrWidgetData.troubleshooting_docs_path = '#{help_page_path('user/project/merge_requests/index.md', anchor: 'troubleshooting')}';
+    window.gl.mrWidgetData.security_approvals_help_page_path = '#{help_page_path('user/application_security/index.html', anchor: 'security-approvals-in-merge-requests-ultimate')}';
+
+#js-vue-mr-widget.mr-widget

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

@@ -14,56 +14,54 @@
   .merge-request-details.issuable-details{ data: { id: @merge_request.project.id } }
     = render "projects/merge_requests/mr_box"
 
-    - if @merge_request.source_branch_exists?
-      = render "projects/merge_requests/how_to_merge"
-
-    = javascript_tag nonce: true do
-      :plain
-        window.gl = window.gl || {};
-        window.gl.mrWidgetData = #{serialize_issuable(@merge_request, serializer: 'widget', issues_links: true)}
-
-        window.gl.mrWidgetData.squash_before_merge_help_path = '#{help_page_path("user/project/merge_requests/squash_and_merge")}';
-        window.gl.mrWidgetData.troubleshooting_docs_path = '#{help_page_path('user/project/merge_requests/index.md', anchor: 'troubleshooting')}';
-        window.gl.mrWidgetData.security_approvals_help_page_path = '#{help_page_path('user/application_security/index.html', anchor: 'security-approvals-in-merge-requests-ultimate')}';
-
-    #js-vue-mr-widget.mr-widget
-
-    .content-block.content-block-small.emoji-list-container.js-noteable-awards
-      = render 'award_emoji/awards_block', awardable: @merge_request, inline: true
+    - 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
-          %li.notes-tab{ data: { qa_selector: 'notes_tab'} }
+          = 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
-              = _("Discussion")
+              - if mr_tabs_position_enabled?
+                = _("Overview")
+              - else
+                = _("Discussion")
               %span.badge.badge-pill= @merge_request.related_notes.user.count
           - if @merge_request.source_project
-            %li.commits-tab
+            = render "projects/merge_requests/tabs/tab", name: "commits", class: "commits-tab" do
               = tab_link_for @merge_request, :commits do
                 = _("Commits")
                 %span.badge.badge-pill= @commits_count
           - if number_of_pipelines.nonzero?
-            %li.pipelines-tab
+            = render "projects/merge_requests/tabs/tab", name: "pipelines", class: "pipelines-tab" do
               = tab_link_for @merge_request, :pipelines do
                 = _("Pipelines")
                 %span.badge.badge-pill.js-pipelines-mr-count= number_of_pipelines
-          %li.diffs-tab.qa-diffs-tab
+          = render "projects/merge_requests/tabs/tab", name: "diffs", class: "diffs-tab qa-diffs-tab", id: "diffs-tab" do
             = 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
-          #js-vue-discussion-filter{ data: { default_filter: current_user&.notes_filter_for(@merge_request),
-            notes_filters: UserPreference.notes_filters.to_json } }
+          - unless mr_tabs_position_enabled?
+            = render "projects/merge_requests/discussion_filter"
           #js-vue-discussion-counter
 
     .tab-content#diff-notes-app
       #js-diff-file-finder
-      #notes.notes.tab-pane.voting_notes
+      = render "projects/merge_requests/tabs/pane", id: "notes", class: "notes voting_notes" do
         .row
           %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"
               #js-vue-mr-discussions{ data: { notes_data: notes_data(@merge_request).to_json,
                 noteable_data: serialize_issuable(@merge_request, serializer: 'noteable'),
                 noteable_type: 'MergeRequest',
@@ -71,12 +69,12 @@
                 help_page_path: suggest_changes_help_path,
                 current_user_data: @current_user_data} }
 
-      #commits.commits.tab-pane
+      = render "projects/merge_requests/tabs/pane", name: "commits", id: "commits", class: "commits" do
         -# This tab is always loaded via AJAX
-      #pipelines.pipelines.tab-pane
+      = render "projects/merge_requests/tabs/pane", name: "pipelines", id: "pipelines", class: "pipelines" do
         - if number_of_pipelines.nonzero?
           = render 'projects/commit/pipelines_list', disable_initialization: true, endpoint: pipelines_project_merge_request_path(@project, @merge_request)
-      #js-diffs-app.diffs.tab-pane{ data: { "is-locked" => @merge_request.discussion_locked?,
+      = render "projects/merge_requests/tabs/pane", name: "diffs", id: "js-diffs-app", class: "diffs", data: { "is-locked": @merge_request.discussion_locked?,
         endpoint: diffs_project_merge_request_path(@project, @merge_request, 'json', request.query_parameters),
         endpoint_metadata: diffs_metadata_project_json_merge_request_path(@project, @merge_request, 'json', request.query_parameters),
         endpoint_batch: diffs_batch_project_json_merge_request_path(@project, @merge_request, 'json', request.query_parameters),
@@ -87,7 +85,7 @@
         is_fluid_layout: fluid_layout.to_s,
         dismiss_endpoint: user_callouts_path,
         show_suggest_popover: show_suggest_popover?.to_s,
-        show_whitespace_default: @show_whitespace_default.to_s } }
+        show_whitespace_default: @show_whitespace_default.to_s }
 
     .mr-loading-status
       = spinner

app/views/projects/merge_requests/tabs/_pane.html.haml

+- tab_name = local_assigns.fetch(:name, nil)
+- tab_id = local_assigns.fetch(:id, nil)
+- tab_class = local_assigns.fetch(:class, nil)
+- tab_data = local_assigns.fetch(:data, nil)
+
+.tab-pane{ id: tab_id, class: tab_class, style: ("display: block" if params[:tab] == tab_name), data: tab_data }
+  = yield

app/views/projects/merge_requests/tabs/_tab.html.haml

+- tab_name = local_assigns.fetch(:name, nil)
+- tab_class = local_assigns.fetch(:class, nil)
+- qa_selector = local_assigns.fetch(:qa_selector, nil)
+- id = local_assigns.fetch(:id, nil)
+
+%li{ class: [tab_class, ("active" if params[:tab] == tab_name)], id: id, data: { qa_selector: qa_selector } }
+  = yield

changelogs/unreleased/ph-33813-moveMergeRequestDescription.yml

+---
+title: Move merge request description into discussions tab
+merge_request: 18940
+author:
+type: changed

doc/user/project/merge_requests/index.md

@@ -40,6 +40,40 @@ B. Consider you're a web developer writing a webpage for your company's website:
 1. Once approved, your merge request is [squashed and merged](squash_and_merge.md), and [deployed to staging with GitLab Pages](https://about.gitlab.com/blog/2016/08/26/ci-deployment-and-environments/)
 1. Your production team [cherry picks](cherry_pick_changes.md) the merge commit into production
 
+## Overview
+
+Merge requests (aka "MRs") display a great deal of information about the changes proposed.
+The body of an MR contains its description, along with its widget (displaying information
+about CI/CD pipelines, when present), followed by the discussion threads of the people
+collaborating with that MR.
+
+MRs also contain navigation tabs from which you can see the discussion happening on the thread,
+the list of commits, the list of pipelines and jobs, the code changes and inline code reviews.
+
+## Merge request navigation tabs at the top
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/33813) in GitLab 12.6. This positioning is experimental.
+
+So far, the navigation tabs present in merge requests to display **Discussion**,
+**Commits**, **Pipelines**, and **Changes** were located after the merge request
+widget.
+
+To facilitate this navigation without having to scroll up and down through the page
+to find these tabs, based on user feedback, we are experimenting with a new positioning
+of these tabs. They are now located at the top of the merge request, with a new
+**Overview** tab, containing the description of the merge request followed by the
+widget. Next to **Overview**, you can find **Pipelines**, **Commits**, and **Changes**.
+
+![Merge request tab positions](img/merge_request_tab_position_v12_6.png)
+
+Please note this change is currently behind a feature flag which is enabled by default. For
+self-managed instances, it can be disabled through the Rails console by a GitLab
+administrator with the following command:
+
+```ruby
+Feature.disable(:mr_tabs_position)
+```
+
 ## Creating merge requests
 
 While making changes to files in the `master` branch of a repository is possible, it is not

locale/gitlab.pot

@@ -11212,6 +11212,9 @@ msgstr ""
 msgid "More information"
 msgstr ""
 
+msgid "More information and share feedback"
+msgstr ""
+
 msgid "More information is available|here"
 msgstr ""
 
@@ -11823,6 +11826,9 @@ msgstr ""
 msgid "November"
 msgstr ""
 
+msgid "Now you can access the merge request navigation tabs at the top, where they’re easier to find."
+msgstr ""
+
 msgid "Number of Elasticsearch replicas"
 msgstr ""
 

spec/features/merge_request/user_sees_merge_request_pipelines_spec.rb

@@ -8,6 +8,7 @@ describe 'Merge request > User sees pipelines triggered by merge request', :js d
 
   let(:project) { create(:project, :public, :repository) }
   let(:user) { project.creator }
+  let(:enable_mr_tabs_position_flag) { true }
 
   let(:config) do
     {
@@ -26,6 +27,7 @@ describe 'Merge request > User sees pipelines triggered by merge request', :js d
   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))
@@ -51,6 +53,7 @@ describe 'Merge request > User sees pipelines triggered by merge request', :js d
       Ci::CreatePipelineService.new(project, user, ref: 'feature')
                                 .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)
@@ -67,9 +70,23 @@ describe 'Merge request > User sees pipelines triggered by merge request', :js d
       end
     end
 
-    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}")
+    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"
+
+        page.within('.ci-widget-content') do
+          expect(page).to have_content("##{detached_merge_request_pipeline.id}")
+        end
       end
     end
 
@@ -243,9 +260,23 @@ describe 'Merge request > User sees pipelines triggered by merge request', :js d
       end
     end
 
-    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}")
+    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"
+
+        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
       end
     end
 
@@ -309,6 +340,8 @@ describe 'Merge request > User sees pipelines triggered by merge request', :js d
       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
@@ -323,6 +356,8 @@ describe 'Merge request > User sees pipelines triggered by merge request', :js d
 
     context 'when a user merges a merge request from a forked project to the parent project' do
       before do
+        click_link("Overview")
+
         click_button 'Merge when pipeline succeeds'
 
         wait_for_requests

spec/features/merge_request/user_sees_mr_with_deleted_source_branch_spec.rb

@@ -25,15 +25,16 @@ describe 'Merge request > User sees MR with deleted source branch', :js do
 
   it 'still contains Discussion, Commits and Changes tabs' do
     within '.merge-request-details' do
-      expect(page).to have_content('Discussion')
+      expect(page).to have_content('Overview')
       expect(page).to have_content('Commits')
       expect(page).to have_content('Changes')
     end
 
+    expect(page).to have_content('Source branch does not exist.')
+
     click_on 'Changes'
     wait_for_requests
 
     expect(page).to have_selector('.diffs.tab-pane .file-holder')
-    expect(page).to have_content('Source branch does not exist.')
   end
 end