Merge branch '197227-load-milestone-issues-tab-async' into 'master'

Load milestone issues tab asynchronously

See merge request gitlab-org/gitlab!42473

app/assets/javascripts/milestone.js

@@ -7,11 +7,6 @@ import { __ } from './locale';
 export default class Milestone {
   constructor() {
     this.bindTabsSwitching();
-
-    // Load merge request tab if it is active
-    // merge request tab is active based on different conditions in the backend
-    this.loadTab($('.js-milestone-tabs .active a'));
-
     this.loadInitialTab();
   }
 
@@ -23,12 +18,14 @@ export default class Milestone {
       this.loadTab($target);
     });
   }
-  // eslint-disable-next-line class-methods-use-this
+
   loadInitialTab() {
-    const $target = $(`.js-milestone-tabs a[href="${window.location.hash}"]`);
+    const $target = $(`.js-milestone-tabs a:not(.active)[href="${window.location.hash}"]`);
 
     if ($target.length) {
       $target.tab('show');
+    } else {
+      this.loadTab($('.js-milestone-tabs a.active'));
     }
   }
   // eslint-disable-next-line class-methods-use-this

app/controllers/concerns/milestone_actions.rb

@@ -3,13 +3,25 @@
 module MilestoneActions
   extend ActiveSupport::Concern
 
+  def issues
+    respond_to do |format|
+      format.html { redirect_to milestone_redirect_path }
+      format.json do
+        render json: tabs_json("shared/milestones/_issues_tab", {
+          issues: @milestone.sorted_issues(current_user), # rubocop:disable Gitlab/ModuleWithInstanceVariables
+          show_project_name: Gitlab::Utils.to_boolean(params[:show_project_name])
+        })
+      end
+    end
+  end
+
   def merge_requests
     respond_to do |format|
       format.html { redirect_to milestone_redirect_path }
       format.json do
         render json: tabs_json("shared/milestones/_merge_requests_tab", {
           merge_requests: @milestone.sorted_merge_requests(current_user), # rubocop:disable Gitlab/ModuleWithInstanceVariables
-          show_project_name: true
+          show_project_name: Gitlab::Utils.to_boolean(params[:show_project_name])
         })
       end
     end

app/controllers/groups/milestones_controller.rb

@@ -3,7 +3,7 @@
 class Groups::MilestonesController < Groups::ApplicationController
   include MilestoneActions
 
-  before_action :milestone, only: [:edit, :show, :update, :merge_requests, :participants, :labels, :destroy]
+  before_action :milestone, only: [:edit, :show, :update, :issues, :merge_requests, :participants, :labels, :destroy]
   before_action :authorize_admin_milestones!, only: [:edit, :new, :create, :update, :destroy]
   before_action do
     push_frontend_feature_flag(:burnup_charts, @group)

app/controllers/projects/milestones_controller.rb

@@ -5,7 +5,7 @@ class Projects::MilestonesController < Projects::ApplicationController
   include MilestoneActions
 
   before_action :check_issuables_available!
-  before_action :milestone, only: [:edit, :update, :destroy, :show, :merge_requests, :participants, :labels, :promote]
+  before_action :milestone, only: [:edit, :update, :destroy, :show, :issues, :merge_requests, :participants, :labels, :promote]
   before_action do
     push_frontend_feature_flag(:burnup_charts, @project)
   end
@@ -14,7 +14,7 @@ class Projects::MilestonesController < Projects::ApplicationController
   before_action :authorize_read_milestone!
 
   # Allow admin milestone
-  before_action :authorize_admin_milestone!, except: [:index, :show, :merge_requests, :participants, :labels]
+  before_action :authorize_admin_milestone!, except: [:index, :show, :issues, :merge_requests, :participants, :labels]
 
   # Allow to promote milestone
   before_action :authorize_promote_milestone!, only: :promote

app/helpers/timeboxes_helper.rb

@@ -228,8 +228,8 @@ module TimeboxesHelper
   end
   alias_method :milestone_date_range, :timebox_date_range
 
-  def milestone_tab_path(milestone, tab)
-    url_for(action: tab, format: :json)
+  def milestone_tab_path(milestone, tab, params = {})
+    url_for(params.merge(action: tab, format: :json))
   end
 
   def update_milestone_path(milestone, params = {})

app/views/shared/milestones/_issuable.html.haml

@@ -10,8 +10,6 @@
   %span
     - if show_project_name
       %strong #{project.name} ·
-    - elsif show_full_project_name
-      %strong #{project.full_name} ·
     - if issuable.is_a?(Issue)
       = confidential_icon(issuable)
     = link_to issuable.title, issuable_url_args, title: issuable.title

app/views/shared/milestones/_issuables.html.haml

@@ -15,4 +15,4 @@
     = render partial: 'shared/milestones/issuable',
              collection: issuables,
              as: :issuable,
-             locals: { show_project_name: show_project_name, show_full_project_name: show_full_project_name }
+             locals: { show_project_name: show_project_name }

app/views/shared/milestones/_issues_tab.html.haml

-- args = { show_project_name: local_assigns.fetch(:show_project_name, false),
-           show_full_project_name: local_assigns.fetch(:show_full_project_name, false) }
+- args = { show_project_name: local_assigns.fetch(:show_project_name, false) }
 
 - if display_issues_count_warning?(@milestone)
   .flash-container

app/views/shared/milestones/_merge_requests_tab.haml

-- args = { show_project_name: local_assigns.fetch(:show_project_name, false),
-           show_full_project_name: local_assigns.fetch(:show_full_project_name, false) }
+- args = { show_project_name: local_assigns.fetch(:show_project_name, false) }
 
 .row.gl-mt-3
   .col-md-3

app/views/shared/milestones/_tabs.html.haml

+- show_project_name = local_assigns.fetch(:show_project_name, false)
+
 .scrolling-tabs-container.inner-page-scroll-tabs.is-smaller
   .fade-left= sprite_icon('chevron-lg-left', size: 12)
   .fade-right= sprite_icon('chevron-lg-right', size: 12)
   %ul.nav-links.scrolling-tabs.js-milestone-tabs.nav.nav-tabs
     %li.nav-item
-      = link_to '#tab-issues', class: 'nav-link active', data: { toggle: 'tab', show: '.tab-issues-buttons' } do
+      = link_to '#tab-issues', class: 'nav-link active', data: { toggle: 'tab', endpoint: milestone_tab_path(milestone, 'issues', show_project_name: show_project_name) } do
         = _('Issues')
         %span.badge.badge-pill= milestone.issues_visible_to_user(current_user).size
     - if milestone.merge_requests_enabled?
       %li.nav-item
-        = link_to '#tab-merge-requests', class: 'nav-link', data: { toggle: 'tab', endpoint: milestone_tab_path(milestone, 'merge_requests') } do
+        = link_to '#tab-merge-requests', class: 'nav-link', data: { toggle: 'tab', endpoint: milestone_tab_path(milestone, 'merge_requests', show_project_name: show_project_name) } do
           = _('Merge Requests')
           %span.badge.badge-pill= milestone.merge_requests_visible_to_user(current_user).size
     %li.nav-item
@@ -20,20 +22,13 @@
         = _('Labels')
         %span.badge.badge-pill= milestone.issue_labels_visible_by_user(current_user).count
 
-- issues = milestone.sorted_issues(current_user)
-- show_project_name = local_assigns.fetch(:show_project_name, false)
-- show_full_project_name = local_assigns.fetch(:show_full_project_name, false)
-
 .tab-content.milestone-content
-  .tab-pane.active#tab-issues{ data: { sort_endpoint: (sort_issues_project_milestone_path(@project, @milestone) if @project && current_user) } }
-    = render 'shared/milestones/issues_tab', issues: issues, show_project_name: show_project_name, show_full_project_name: show_full_project_name
+  .tab-pane.active#tab-issues
+    = render "shared/milestones/tab_loading"
   - if milestone.merge_requests_enabled?
     .tab-pane#tab-merge-requests
-      -# loaded async
       = render "shared/milestones/tab_loading"
   .tab-pane#tab-participants
-    -# loaded async
     = render "shared/milestones/tab_loading"
   .tab-pane#tab-labels
-    -# loaded async
     = render "shared/milestones/tab_loading"

changelogs/unreleased/197227-load-milestone-issues-tab-async.yml

+---
+title: Load issues tab in the milestone page asynchronously
+merge_request: 42473
+author:
+type: performance

config/routes/group.rb

@@ -61,6 +61,7 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
 
     resources :milestones, constraints: { id: %r{[^/]+} } do
       member do
+        get :issues
         get :merge_requests
         get :participants
         get :labels

config/routes/project.rb

@@ -161,8 +161,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
         resources :milestones, constraints: { id: /\d+/ } do
           member do
             post :promote
-            put :sort_issues
-            put :sort_merge_requests
+            get :issues
             get :merge_requests
             get :participants
             get :labels

spec/controllers/groups/milestones_controller_spec.rb

@@ -9,7 +9,6 @@ RSpec.describe Groups::MilestonesController do
   let(:user)    { create(:user) }
   let(:title) { '肯定不是中文的问题' }
   let(:milestone) { create(:milestone, project: project) }
-  let(:milestone_path) { group_milestone_path(group, milestone.safe_title, title: milestone.title) }
 
   let(:milestone_params) do
     {
@@ -25,6 +24,12 @@ RSpec.describe Groups::MilestonesController do
     project.add_maintainer(user)
   end
 
+  it_behaves_like 'milestone tabs' do
+    let(:milestone) { create(:milestone, group: group) }
+    let(:milestone_path) { group_milestone_path(group, milestone.iid) }
+    let(:request_params) { { group_id: group, id: milestone.iid } }
+  end
+
   describe '#index' do
     describe 'as HTML' do
       render_views

spec/controllers/projects/milestones_controller_spec.rb

@@ -17,7 +17,9 @@ RSpec.describe Projects::MilestonesController do
     controller.instance_variable_set(:@project, project)
   end
 
-  it_behaves_like 'milestone tabs'
+  it_behaves_like 'milestone tabs' do
+    let(:request_params) { { namespace_id: project.namespace, project_id: project, id: milestone.iid } }
+  end
 
   describe "#show" do
     render_views

spec/features/milestones/user_views_milestone_spec.rb

@@ -4,12 +4,16 @@ require 'spec_helper'
 
 RSpec.describe "User views milestone" do
   let_it_be(:user) { create(:user) }
-  let_it_be(:project) { create(:project) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:project) { create(:project, :repository, group: group) }
   let_it_be(:milestone) { create(:milestone, project: project) }
   let_it_be(:labels) { create_list(:label, 2, project: project) }
 
-  before do
+  before_all do
     project.add_developer(user)
+  end
+
+  before do
     sign_in(user)
   end
 
@@ -25,7 +29,7 @@ RSpec.describe "User views milestone" do
     expect { visit_milestone }.not_to exceed_query_limit(control)
   end
 
-  context 'limiting milestone issues' do
+  context 'issues list', :js do
     before_all do
       2.times do
         create(:issue, milestone: milestone, project: project)
@@ -34,6 +38,28 @@ RSpec.describe "User views milestone" do
       end
     end
 
+    context 'for a project milestone' do
+      it 'does not show the project name' do
+        visit(project_milestone_path(project, milestone))
+
+        wait_for_requests
+
+        expect(page.find('#tab-issues')).not_to have_text(project.name)
+      end
+    end
+
+    context 'for a group milestone' do
+      let(:group_milestone) { create(:milestone, group: group) }
+
+      it 'shows the project name' do
+        create(:issue, project: project, milestone: group_milestone)
+
+        visit(group_milestone_path(group, group_milestone))
+
+        expect(page.find('#tab-issues')).to have_text(project.name)
+      end
+    end
+
     context 'when issues on milestone are over DISPLAY_ISSUES_LIMIT' do
       it "limits issues to display and shows warning" do
         stub_const('Milestoneish::DISPLAY_ISSUES_LIMIT', 3)
@@ -56,6 +82,40 @@ RSpec.describe "User views milestone" do
     end
   end
 
+  context 'merge requests list', :js do
+    context 'for a project milestone' do
+      it 'does not show the project name' do
+        create(:merge_request, source_project: project, milestone: milestone)
+
+        visit(project_milestone_path(project, milestone))
+
+        within('.js-milestone-tabs') do
+          click_link('Merge Requests')
+        end
+
+        wait_for_requests
+
+        expect(page.find('#tab-merge-requests')).not_to have_text(project.name)
+      end
+    end
+
+    context 'for a group milestone' do
+      let(:group_milestone) { create(:milestone, group: group) }
+
+      it 'shows the project name' do
+        create(:merge_request, source_project: project, milestone: group_milestone)
+
+        visit(group_milestone_path(group, group_milestone))
+
+        within('.js-milestone-tabs') do
+          click_link('Merge Requests')
+        end
+
+        expect(page.find('#tab-merge-requests')).to have_text(project.name)
+      end
+    end
+  end
+
   private
 
   def visit_milestone

spec/features/milestones/user_views_milestones_spec.rb

@@ -21,7 +21,7 @@ RSpec.describe "User views milestones" do
       .and have_content("Merge Requests")
   end
 
-  context "with issues" do
+  context "with issues", :js do
     let_it_be(:issue) { create(:issue, project: project, milestone: milestone) }
     let_it_be(:closed_issue) { create(:closed_issue, project: project, milestone: milestone) }
 
@@ -33,7 +33,6 @@ RSpec.describe "User views milestones" do
         .and have_selector("#tab-issues li.issuable-row", count: 2)
         .and have_content(issue.title)
         .and have_content(closed_issue.title)
-        .and have_selector("#tab-merge-requests")
     end
   end
 

spec/support/shared_examples/controllers/milestone_tabs_shared_examples.rb

@@ -2,9 +2,28 @@
 
 RSpec.shared_examples 'milestone tabs' do
   def go(path, extra_params = {})
-    params = { namespace_id: project.namespace.to_param, project_id: project, id: milestone.iid }
+    get path, params: request_params.merge(extra_params)
+  end
+
+  describe '#issues' do
+    context 'as html' do
+      before do
+        go(:issues, format: 'html')
+      end
 
-    get path, params: params.merge(extra_params)
+      it 'redirects to milestone#show' do
+        expect(response).to redirect_to(milestone_path)
+      end
+    end
+
+    context 'as json' do
+      it 'renders the issues tab template to a string' do
+        go(:issues, format: 'json')
+
+        expect(response).to render_template('shared/milestones/_issues_tab')
+        expect(json_response).to have_key('html')
+      end
+    end
   end
 
   describe '#merge_requests' do

spec/views/shared/milestones/_issuables.html.haml_spec.rb

@@ -6,8 +6,7 @@ RSpec.describe 'shared/milestones/_issuables.html.haml' do
   let(:issuables_size) { 100 }
 
   before do
-    allow(view).to receive_messages(title: nil, id: nil, show_project_name: nil,
-                                    show_full_project_name: nil, dom_class: '',
+    allow(view).to receive_messages(title: nil, id: nil, show_project_name: nil, dom_class: '',
                                     issuables: double(length: issuables_size).as_null_object)
 
     stub_template 'shared/milestones/_issuable.html.haml' => ''