Merge branch 'async-milestone-tabs' into 'master'

Load milestone tabs asynchronously

See merge request !10919

18 files changed, 266 insertions, 43 deletions

diff --git a/app/assets/javascripts/milestone.js b/app/assets/javascripts/milestone.js
index 38c673e8907..841b24a60a3 100644
--- a/app/assets/javascripts/milestone.js
+++ b/app/assets/javascripts/milestone.js
@@ -19,12 +19,10 @@
       });
     };
 
-    Milestone.sortIssues = function(data) {
-      var sort_issues_url;
-      sort_issues_url = location.href + "/sort_issues";
+    Milestone.sortIssues = function(url, data) {
       return $.ajax({
         type: "PUT",
-        url: sort_issues_url,
+        url,
         data: data,
         success: function(_data) {
           return Milestone.successCallback(_data);
@@ -36,12 +34,10 @@
       });
     };
 
-    Milestone.sortMergeRequests = function(data) {
-      var sort_mr_url;
-      sort_mr_url = location.href + "/sort_merge_requests";
+    Milestone.sortMergeRequests = function(url, data) {
       return $.ajax({
         type: "PUT",
-        url: sort_mr_url,
+        url,
         data: data,
         success: function(_data) {
           return Milestone.successCallback(_data);
@@ -81,42 +77,55 @@
     };
 
     function Milestone() {
-      var oldMouseStart;
+      this.issuesSortEndpoint = $('#tab-issues').data('sort-endpoint');
+      this.mergeRequestsSortEndpoint = $('#tab-merge-requests').data('sort-endpoint');
+
       this.bindIssuesSorting();
-      this.bindMergeRequestSorting();
       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();
     }
 
     Milestone.prototype.bindIssuesSorting = function() {
+      if (!this.issuesSortEndpoint) return;
+
       $('#issues-list-unassigned, #issues-list-ongoing, #issues-list-closed').each(function (i, el) {
         this.createSortable(el, {
           group: 'issue-list',
           listEls: $('.issues-sortable-list'),
           fieldName: 'issue',
-          sortCallback: Milestone.sortIssues,
+          sortCallback: (data) => {
+            Milestone.sortIssues(this.issuesSortEndpoint, data);
+          },
           updateCallback: Milestone.updateIssue,
         });
       }.bind(this));
     };
 
     Milestone.prototype.bindTabsSwitching = function() {
-      return $('a[data-toggle="tab"]').on('show.bs.tab', function(e) {
-        var currentTabClass, previousTabClass;
-        currentTabClass = $(e.target).data('show');
-        previousTabClass = $(e.relatedTarget).data('show');
-        $(previousTabClass).hide();
-        $(currentTabClass).removeClass('hidden');
-        return $(currentTabClass).show();
+      return $('a[data-toggle="tab"]').on('show.bs.tab', (e) => {
+        const $target = $(e.target);
+
+        location.hash = $target.attr('href');
+        this.loadTab($target);
       });
     };
 
     Milestone.prototype.bindMergeRequestSorting = function() {
+      if (!this.mergeRequestsSortEndpoint) return;
+
       $("#merge_requests-list-unassigned, #merge_requests-list-ongoing, #merge_requests-list-closed").each(function (i, el) {
         this.createSortable(el, {
           group: 'merge-request-list',
           listEls: $(".merge_requests-sortable-list:not(#merge_requests-list-merged)"),
           fieldName: 'merge_request',
-          sortCallback: Milestone.sortMergeRequests,
+          sortCallback: (data) => {
+            Milestone.sortMergeRequests(this.mergeRequestsSortEndpoint, data);
+          },
           updateCallback: Milestone.updateMergeRequest,
         });
       }.bind(this));
@@ -169,6 +178,35 @@
       });
     };
 
+    Milestone.prototype.loadInitialTab = function() {
+      const $target = $(`.js-milestone-tabs a[href="${location.hash}"]`);
+
+      if ($target.length) {
+        $target.tab('show');
+      }
+    };
+
+    Milestone.prototype.loadTab = function($target) {
+      const endpoint = $target.data('endpoint');
+      const tabElId = $target.attr('href');
+
+      if (endpoint && !$target.hasClass('is-loaded')) {
+        $.ajax({
+          url: endpoint,
+          dataType: 'JSON',
+        })
+        .fail(() => new Flash('Error loading milestone tab'))
+        .done((data) => {
+          $(tabElId).html(data.html);
+          $target.addClass('is-loaded');
+
+          if (tabElId === '#tab-merge-requests') {
+            this.bindMergeRequestSorting();
+          }
+        });
+      }
+    };
+
     return Milestone;
   })();
 }).call(window);
diff --git a/app/controllers/concerns/milestone_actions.rb b/app/controllers/concerns/milestone_actions.rb
new file mode 100644
index 00000000000..3e2a0fe4f8b
--- /dev/null
+++ b/app/controllers/concerns/milestone_actions.rb
@@ -0,0 +1,53 @@
+module MilestoneActions
+  extend ActiveSupport::Concern
+
+  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.merge_requests,
+          show_project_name: true
+        })
+      end
+    end
+  end
+
+  def participants
+    respond_to do |format|
+      format.html { redirect_to milestone_redirect_path }
+      format.json do
+        render json: tabs_json("shared/milestones/_participants_tab", {
+          users: @milestone.participants
+        })
+      end
+    end
+  end
+
+  def labels
+    respond_to do |format|
+      format.html { redirect_to milestone_redirect_path }
+      format.json do
+        render json: tabs_json("shared/milestones/_labels_tab", {
+          labels: @milestone.labels
+        })
+      end
+    end
+  end
+
+  private
+
+  def tabs_json(partial, data = {})
+    {
+      html: view_to_html_string(partial, data)
+    }
+  end
+
+  def milestone_redirect_path
+    if @project
+      namespace_project_milestone_path(@project.namespace, @project, @milestone)
+    else
+      group_milestone_path(@group, @milestone.safe_title, title: @milestone.title)
+    end
+  end
+end
diff --git a/app/controllers/groups/milestones_controller.rb b/app/controllers/groups/milestones_controller.rb
index 43102596201..e52fa766044 100644
--- a/app/controllers/groups/milestones_controller.rb
+++ b/app/controllers/groups/milestones_controller.rb
@@ -1,6 +1,8 @@
 class Groups::MilestonesController < Groups::ApplicationController
+  include MilestoneActions
+
   before_action :group_projects
-  before_action :milestone, only: [:show, :update]
+  before_action :milestone, only: [:show, :update, :merge_requests, :participants, :labels]
   before_action :authorize_admin_milestones!, only: [:new, :create, :update]
 
   def index
diff --git a/app/controllers/projects/milestones_controller.rb b/app/controllers/projects/milestones_controller.rb
index d0dd524c484..c56bce19eee 100644
--- a/app/controllers/projects/milestones_controller.rb
+++ b/app/controllers/projects/milestones_controller.rb
@@ -1,12 +1,14 @@
 class Projects::MilestonesController < Projects::ApplicationController
+  include MilestoneActions
+
   before_action :module_enabled
-  before_action :milestone, only: [:edit, :update, :destroy, :show, :sort_issues, :sort_merge_requests]
+  before_action :milestone, only: [:edit, :update, :destroy, :show, :sort_issues, :sort_merge_requests, :merge_requests, :participants, :labels]
 
   # Allow read any milestone
   before_action :authorize_read_milestone!
 
   # Allow admin milestone
-  before_action :authorize_admin_milestone!, except: [:index, :show]
+  before_action :authorize_admin_milestone!, except: [:index, :show, :merge_requests, :participants, :labels]
 
   respond_to :html
 
diff --git a/app/helpers/milestones_helper.rb b/app/helpers/milestones_helper.rb
index c9e70faa52e..c515774140c 100644
--- a/app/helpers/milestones_helper.rb
+++ b/app/helpers/milestones_helper.rb
@@ -115,4 +115,28 @@ module MilestonesHelper
       end
     end
   end
+
+  def milestone_merge_request_tab_path(milestone)
+    if @project
+      merge_requests_namespace_project_milestone_path(@project.namespace, @project, milestone, format: :json)
+    elsif @group
+      merge_requests_group_milestone_path(@group, milestone.safe_title, title: milestone.title, format: :json)
+    end
+  end
+
+  def milestone_participants_tab_path(milestone)
+    if @project
+      participants_namespace_project_milestone_path(@project.namespace, @project, milestone, format: :json)
+    elsif @group
+      participants_group_milestone_path(@group, milestone.safe_title, title: milestone.title, format: :json)
+    end
+  end
+
+  def milestone_labels_tab_path(milestone)
+    if @project
+      labels_namespace_project_milestone_path(@project.namespace, @project, milestone, format: :json)
+    elsif @group
+      labels_group_milestone_path(@group, milestone.safe_title, title: milestone.title, format: :json)
+    end
+  end
 end
diff --git a/app/views/shared/milestones/_tab_loading.html.haml b/app/views/shared/milestones/_tab_loading.html.haml
new file mode 100644
index 00000000000..68458c2d0aa
--- /dev/null
+++ b/app/views/shared/milestones/_tab_loading.html.haml
@@ -0,0 +1,2 @@
+.text-center.prepend-top-default
+  = icon('spin spinner 2x', 'aria-hidden': 'true', 'aria-label': 'Loading tab content')
diff --git a/app/views/shared/milestones/_tabs.html.haml b/app/views/shared/milestones/_tabs.html.haml
index 9a4502873ef..6a6d817b344 100644
--- a/app/views/shared/milestones/_tabs.html.haml
+++ b/app/views/shared/milestones/_tabs.html.haml
@@ -1,27 +1,27 @@
 .scrolling-tabs-container.inner-page-scroll-tabs.is-smaller
   .fade-left= icon('angle-left')
   .fade-right= icon('angle-right')
-  %ul.nav-links.scrolling-tabs
+  %ul.nav-links.scrolling-tabs.js-milestone-tabs
     - if milestone.is_a?(GlobalMilestone) || can?(current_user, :read_issue, @project)
       %li.active
         = link_to '#tab-issues', 'data-toggle' => 'tab', 'data-show' => '.tab-issues-buttons' do
           Issues
           %span.badge= milestone.issues_visible_to_user(current_user).size
       %li
-        = link_to '#tab-merge-requests', 'data-toggle' => 'tab', 'data-show' => '.tab-merge-requests-buttons' do
+        = link_to '#tab-merge-requests', 'data-toggle' => 'tab', 'data-endpoint': milestone_merge_request_tab_path(milestone) do
           Merge Requests
           %span.badge= milestone.merge_requests.size
     - else
       %li.active
-        = link_to '#tab-merge-requests', 'data-toggle' => 'tab', 'data-show' => '.tab-merge-requests-buttons' do
+        = link_to '#tab-merge-requests', 'data-toggle' => 'tab', 'data-endpoint': milestone_merge_request_tab_path(milestone) do
           Merge Requests
           %span.badge= milestone.merge_requests.size
     %li
-      = link_to '#tab-participants', 'data-toggle' => 'tab' do
+      = link_to '#tab-participants', 'data-toggle' => 'tab', 'data-endpoint': milestone_participants_tab_path(milestone) do
         Participants
         %span.badge= milestone.participants.count
     %li
-      = link_to '#tab-labels', 'data-toggle' => 'tab' do
+      = link_to '#tab-labels', 'data-toggle' => 'tab', 'data-endpoint': milestone_labels_tab_path(milestone) do
         Labels
         %span.badge= milestone.labels.count
 
@@ -30,14 +30,18 @@
 
 .tab-content.milestone-content
   - if milestone.is_a?(GlobalMilestone) || can?(current_user, :read_issue, @project)
-    .tab-pane.active#tab-issues
+    .tab-pane.active#tab-issues{ data: { sort_endpoint: (sort_issues_namespace_project_milestone_path(@project.namespace, @project, @milestone) if @project && current_user) } }
       = render 'shared/milestones/issues_tab', issues: milestone.issues_visible_to_user(current_user).include_associations, show_project_name: show_project_name, show_full_project_name: show_full_project_name
-    .tab-pane#tab-merge-requests
-      = render 'shared/milestones/merge_requests_tab', merge_requests: milestone.merge_requests, show_project_name: show_project_name, show_full_project_name: show_full_project_name
+    .tab-pane#tab-merge-requests{ data: { sort_endpoint: (sort_merge_requests_namespace_project_milestone_path(@project.namespace, @project, @milestone) if @project && current_user) } }
+      -# loaded async
+      = render "shared/milestones/tab_loading"
   - else
-    .tab-pane.active#tab-merge-requests
-      = render 'shared/milestones/merge_requests_tab', merge_requests: milestone.merge_requests, show_project_name: show_project_name, show_full_project_name: show_full_project_name
+    .tab-pane.active#tab-merge-requests{ data: { sort_endpoint: (sort_merge_requests_namespace_project_milestone_path(@project.namespace, @project, @milestone) if @project && current_user) } }
+      -# loaded async
+      = render "shared/milestones/tab_loading"
   .tab-pane#tab-participants
-    = render 'shared/milestones/participants_tab', users: milestone.participants
+    -# loaded async
+    = render "shared/milestones/tab_loading"
   .tab-pane#tab-labels
-    = render 'shared/milestones/labels_tab', labels: milestone.labels
+    -# loaded async
+    = render "shared/milestones/tab_loading"
diff --git a/changelogs/unreleased/async-milestone-tabs.yml b/changelogs/unreleased/async-milestone-tabs.yml
new file mode 100644
index 00000000000..c199a95610c
--- /dev/null
+++ b/changelogs/unreleased/async-milestone-tabs.yml
@@ -0,0 +1,4 @@
+---
+title: Load milestone tabs asynchronously to increase initial load performance
+merge_request:
+author:
diff --git a/config/routes/group.rb b/config/routes/group.rb
index 73f69d76995..7b29e0e807c 100644
--- a/config/routes/group.rb
+++ b/config/routes/group.rb
@@ -10,7 +10,13 @@ scope(path: 'groups/*group_id',
   end
 
   resource :avatar, only: [:destroy]
-  resources :milestones, constraints: { id: /[^\/]+/ }, only: [:index, :show, :update, :new, :create]
+  resources :milestones, constraints: { id: /[^\/]+/ }, only: [:index, :show, :update, :new, :create] do
+    member do
+      get :merge_requests
+      get :participants
+      get :labels
+    end
+  end
 
   resources :labels, except: [:show] do
     post :toggle_subscription, on: :member
diff --git a/config/routes/project.rb b/config/routes/project.rb
index 894faeb6188..a15e365cc2f 100644
--- a/config/routes/project.rb
+++ b/config/routes/project.rb
@@ -207,6 +207,9 @@ constraints(ProjectUrlConstrainer.new) do
         member do
           put :sort_issues
           put :sort_merge_requests
+          get :merge_requests
+          get :participants
+          get :labels
         end
       end
 
diff --git a/features/group/milestones.feature b/features/group/milestones.feature
index d6c05df9840..1c1539b3e12 100644
--- a/features/group/milestones.feature
+++ b/features/group/milestones.feature
@@ -38,6 +38,7 @@ Feature: Group Milestones
     And I should see the "feature" label
     And I should see the project name in the Issue row
 
+  @javascript
   Scenario: I should see the Labels tab
     Given Group has projects with milestones
     When I visit group "Owned" page
diff --git a/features/project/milestone.feature b/features/project/milestone.feature
index 713f0f3b979..5e7b211fa27 100644
--- a/features/project/milestone.feature
+++ b/features/project/milestone.feature
@@ -7,14 +7,6 @@ Feature: Project Milestone
     And milestone has issue "Bugfix1" with labels: "bug", "feature"
     And milestone has issue "Bugfix2" with labels: "bug", "enhancement"
 
-
-  @javascript
-  Scenario: Listing issues from issues tab
-    Given I visit project "Shop" milestones page
-    And I click link "v2.2"
-    Then I should see the labels "bug", "enhancement" and "feature"
-    And I should see the "bug" label listed only once
-
   @javascript
   Scenario: Listing labels from labels tab
     Given I visit project "Shop" milestones page
diff --git a/features/steps/group/milestones.rb b/features/steps/group/milestones.rb
index f8f5e3f2382..49fcd6f1201 100644
--- a/features/steps/group/milestones.rb
+++ b/features/steps/group/milestones.rb
@@ -1,4 +1,5 @@
 class Spinach::Features::GroupMilestones < Spinach::FeatureSteps
+  include WaitForAjax
   include SharedAuthentication
   include SharedPaths
   include SharedGroup
@@ -90,6 +91,8 @@ class Spinach::Features::GroupMilestones < Spinach::FeatureSteps
   end
 
   step 'I should see the list of labels' do
+    wait_for_ajax
+
     page.within('#tab-labels') do
       expect(page).to have_content 'bug'
       expect(page).to have_content 'feature'
diff --git a/features/steps/project/project_milestone.rb b/features/steps/project/project_milestone.rb
index 1864b3a2b52..dc1190b7eea 100644
--- a/features/steps/project/project_milestone.rb
+++ b/features/steps/project/project_milestone.rb
@@ -2,6 +2,7 @@ class Spinach::Features::ProjectMilestone < Spinach::FeatureSteps
   include SharedAuthentication
   include SharedProject
   include SharedPaths
+  include WaitForAjax
 
   step 'milestone has issue "Bugfix1" with labels: "bug", "feature"' do
     project = Project.find_by(name: "Shop")
@@ -34,6 +35,8 @@ class Spinach::Features::ProjectMilestone < Spinach::FeatureSteps
   end
 
   step 'I should see the labels "bug", "enhancement" and "feature"' do
+    wait_for_ajax
+
     page.within('#tab-issues') do
       expect(page).to have_content 'bug'
       expect(page).to have_content 'enhancement'
diff --git a/spec/controllers/groups/milestones_controller_spec.rb b/spec/controllers/groups/milestones_controller_spec.rb
index 6e4b5f78e33..7cf2996ffd0 100644
--- a/spec/controllers/groups/milestones_controller_spec.rb
+++ b/spec/controllers/groups/milestones_controller_spec.rb
@@ -6,6 +6,16 @@ describe Groups::MilestonesController do
   let(:project2) { create(:empty_project, group: group) }
   let(:user)    { create(:user) }
   let(:title) { '肯定不是中文的问题' }
+  let(:milestone) do
+    project_milestone = create(:milestone, project: project)
+
+    GroupMilestone.build(
+      group,
+      [project],
+      project_milestone.title
+    )
+  end
+  let(:milestone_path) { group_milestone_path(group, milestone.safe_title, title: milestone.title) }
 
   before do
     sign_in(user)
@@ -14,6 +24,8 @@ describe Groups::MilestonesController do
     controller.instance_variable_set(:@group, group)
   end
 
+  it_behaves_like 'milestone tabs'
+
   describe "#create" do
     it "creates group milestone with Chinese title" do
       post :create,
diff --git a/spec/controllers/projects/milestones_controller_spec.rb b/spec/controllers/projects/milestones_controller_spec.rb
index 47e61c3cea8..84a61b2784e 100644
--- a/spec/controllers/projects/milestones_controller_spec.rb
+++ b/spec/controllers/projects/milestones_controller_spec.rb
@@ -7,6 +7,7 @@ describe Projects::MilestonesController do
   let(:issue) { create(:issue, project: project, milestone: milestone) }
   let!(:label) { create(:label, project: project, title: 'Issue Label', issues: [issue]) }
   let!(:merge_request) { create(:merge_request, source_project: project, target_project: project, milestone: milestone) }
+  let(:milestone_path) { namespace_project_milestone_path }
 
   before do
     sign_in(user)
@@ -14,6 +15,8 @@ describe Projects::MilestonesController do
     controller.instance_variable_set(:@project, project)
   end
 
+  it_behaves_like 'milestone tabs'
+
   describe "#show" do
     render_views
 
diff --git a/spec/features/milestones/milestones_spec.rb b/spec/features/milestones/milestones_spec.rb
index 50d7ca39045..9eec3d7f270 100644
--- a/spec/features/milestones/milestones_spec.rb
+++ b/spec/features/milestones/milestones_spec.rb
@@ -86,6 +86,9 @@ describe 'Milestone draggable', feature: true, js: true do
 
     visit namespace_project_milestone_path(project.namespace, project, milestone)
     page.find("a[href='#tab-merge-requests']").click
+
+    wait_for_ajax
+
     scroll_into_view('.milestone-content')
     drag_to(selector: '.merge_requests-sortable-list', list_to_index: 1)
 
diff --git a/spec/support/milestone_tabs_examples.rb b/spec/support/milestone_tabs_examples.rb
new file mode 100644
index 00000000000..c69f8e11008
--- /dev/null
+++ b/spec/support/milestone_tabs_examples.rb
@@ -0,0 +1,68 @@
+shared_examples 'milestone tabs' do
+  def go(path, extra_params = {})
+    params = if milestone.is_a?(GlobalMilestone)
+               { group_id: group.id, id: milestone.safe_title, title: milestone.title }
+             else
+               { namespace_id: project.namespace.to_param, project_id: project, id: milestone.iid }
+             end
+
+    get path, params.merge(extra_params)
+  end
+
+  describe '#merge_requests' do
+    context 'as html' do
+      before { go(:merge_requests, format: 'html') }
+
+      it 'redirects to milestone#show' do
+        expect(response).to redirect_to(milestone_path)
+      end
+    end
+
+    context 'as json' do
+      before { go(:merge_requests, format: 'json') }
+
+      it 'renders the merge requests tab template to a string' do
+        expect(response).to render_template('shared/milestones/_merge_requests_tab')
+        expect(json_response).to have_key('html')
+      end
+    end
+  end
+
+  describe '#participants' do
+    context 'as html' do
+      before { go(:participants, format: 'html') }
+
+      it 'redirects to milestone#show' do
+        expect(response).to redirect_to(milestone_path)
+      end
+    end
+
+    context 'as json' do
+      before { go(:participants, format: 'json') }
+
+      it 'renders the participants tab template to a string' do
+        expect(response).to render_template('shared/milestones/_participants_tab')
+        expect(json_response).to have_key('html')
+      end
+    end
+  end
+
+  describe '#labels' do
+    context 'as html' do
+      before { go(:labels, format: 'html') }
+
+      it 'redirects to milestone#show' do
+        expect(response).to redirect_to(milestone_path)
+      end
+    end
+
+    context 'as json' do
+      before { go(:labels, format: 'json') }
+
+      it 'renders the labels tab template to a string' do
+        expect(response).to render_template('shared/milestones/_labels_tab')
+        expect(json_response).to have_key('html')
+      end
+    end
+  end
+end