Merge branch 'group_milestones_n_1' into 'master'

Fix N+1 in Group milestone view

See merge request gitlab-org/gitlab!26051

app/controllers/projects/milestones_controller.rb

@@ -24,7 +24,6 @@ class Projects::MilestonesController < Projects::ApplicationController
 
     respond_to do |format|
       format.html do
-        @project_namespace = @project.namespace.becomes(Namespace)
         # We need to show group milestones in the JSON response
         # so that people can filter by and assign group milestones,
         # but we don't need to show them on the project milestones page itself.
@@ -47,8 +46,6 @@ class Projects::MilestonesController < Projects::ApplicationController
   end
 
   def show
-    @project_namespace = @project.namespace.becomes(Namespace)
-
     respond_to do |format|
       format.html
     end

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

 -# @project is present when viewing Project's milestone
 - project = @project || issuable.project
-- namespace = @project_namespace || project.namespace.becomes(Namespace)
 - labels = issuable.labels
 - assignees = issuable.assignees
-- base_url_args = [namespace, project]
+- base_url_args = [project]
 - issuable_type_args = base_url_args + [issuable.class.table_name]
 - issuable_url_args = base_url_args + [issuable]
 
@@ -17,7 +16,7 @@
       = confidential_icon(issuable)
     = link_to issuable.title, issuable_url_args, title: issuable.title
   .issuable-detail
-    = link_to [namespace, project, issuable], class: 'issue-link' do
+    = link_to issuable_url_args, class: 'issue-link' do
       %span.issuable-number= issuable.to_reference
 
     - labels.each do |label|

changelogs/unreleased/group_milestones_n_1.yml

+---
+title: Fix N+1 in Group milestone view
+merge_request: 26051
+author:
+type: performance

spec/requests/groups/milestones_controller_spec.rb

@@ -12,23 +12,45 @@ describe Groups::MilestonesController do
     end
     let!(:private_milestone) { create(:milestone, project: public_project_with_private_issues_and_mrs, title: 'project milestone') }
 
-    it 'avoids N+1 database queries' do
-      public_project = create(:project, :public, :merge_requests_enabled, :issues_enabled, group: public_group)
-      create(:milestone, project: public_project)
+    describe 'GET #index' do
+      it 'avoids N+1 database queries' do
+        public_project = create(:project, :public, :merge_requests_enabled, :issues_enabled, group: public_group)
+        create(:milestone, project: public_project)
 
-      control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { get "/groups/#{public_group.to_param}/-/milestones.json" }.count
+        control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { get group_milestones_path(public_group, format: :json) }.count
 
-      projects = create_list(:project, 2, :public, :merge_requests_enabled, :issues_enabled, group: public_group)
-      projects.each do |project|
-        create(:milestone, project: project)
+        projects = create_list(:project, 2, :public, :merge_requests_enabled, :issues_enabled, group: public_group)
+        projects.each do |project|
+          create(:milestone, project: project)
+        end
+
+        expect { get group_milestones_path(public_group, format: :json) }.not_to exceed_all_query_limit(control_count)
+        expect(response).to have_gitlab_http_status(:ok)
+        milestones = json_response
+
+        expect(milestones.count).to eq(3)
+        expect(milestones.map {|x| x['title']}).not_to include(private_milestone.title)
       end
+    end
 
-      expect { get "/groups/#{public_group.to_param}/-/milestones.json" }.not_to exceed_all_query_limit(control_count)
-      expect(response).to have_gitlab_http_status(:ok)
-      milestones = json_response
+    describe 'GET #show' do
+      let(:milestone) { create(:milestone, group: public_group) }
+      let(:show_path) { group_milestone_path(public_group, milestone) }
 
-      expect(milestones.count).to eq(3)
-      expect(milestones.map {|x| x['title']}).not_to include(private_milestone.title)
+      it 'avoids N+1 database queries' do
+        projects = create_list(:project, 3, :public, :merge_requests_enabled, :issues_enabled, group: public_group)
+        projects.each do |project|
+          create_list(:issue, 2, milestone: milestone, project: project)
+        end
+        control = ActiveRecord::QueryRecorder.new(skip_cached: false) { get show_path }
+
+        projects = create_list(:project, 3, :public, :merge_requests_enabled, :issues_enabled, group: public_group)
+        projects.each do |project|
+          create_list(:issue, 2, milestone: milestone, project: project)
+        end
+
+        expect { get show_path }.not_to exceed_all_query_limit(control)
+      end
     end
   end
 end

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

@@ -3,19 +3,38 @@
 require 'spec_helper'
 
 describe 'shared/milestones/_issuable.html.haml' do
-  let(:project) { create(:project) }
-  let(:user) { create(:user) }
-  let(:milestone) { create(:milestone, project: project) }
-  let(:issuable) { create(:issue, project: project, assignees: [user]) }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:milestone) { create(:milestone, project: project) }
 
   before do
     assign(:project, project)
     assign(:milestone, milestone)
   end
 
-  it 'avatar links to issues page' do
-    render 'shared/milestones/issuable', issuable: issuable, show_project_name: true
+  subject(:rendered) { render 'shared/milestones/issuable', issuable: issuable, show_project_name: true }
 
-    expect(rendered).to have_css("a[href='#{project_issues_path(project, milestone_title: milestone.title, assignee_id: user.id, state: 'all')}']")
+  context 'issue' do
+    let(:issuable) { create(:issue, project: project, assignees: [user]) }
+
+    it 'links to the page for the issue' do
+      expect(rendered).to have_css("a[href='#{project_issue_path(project, issuable)}']", class: 'issue-link')
+    end
+
+    it 'links to issues page for user' do
+      expect(rendered).to have_css("a[href='#{project_issues_path(project, milestone_title: milestone.title, assignee_id: user.id, state: 'all')}']")
+    end
+  end
+
+  context 'merge request' do
+    let(:issuable) { create(:merge_request, source_project: project, target_project: project, assignees: [user]) }
+
+    it 'links to merge requests page for user' do
+      expect(rendered).to have_css("a[href='#{project_merge_requests_path(project, milestone_title: milestone.title, assignee_id: user.id, state: 'all')}']")
+    end
+
+    it 'links to the page for the merge request' do
+      expect(rendered).to have_css("a[href='#{project_merge_request_path(project, issuable)}']", class: 'issue-link')
+    end
   end
 end