Merge branch 'sh-optimize-milestone-polymorphic-url' into 'master'

Eliminate N+1 queries in loading namespaces for every issuable in milestones

See merge request !10871

app/controllers/projects/milestones_controller.rb

@@ -23,6 +23,7 @@ class Projects::MilestonesController < Projects::ApplicationController
 
     respond_to do |format|
       format.html do
+        @project_namespace = @project.namespace.becomes(Namespace)
         @milestones = @milestones.includes(:project)
         @milestones = @milestones.page(params[:page])
       end

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

@@ -1,11 +1,14 @@
 -# @project is present when viewing Project's milestone
 - project = @project || issuable.project
+- namespace = @project_namespace || project.namespace.becomes(Namespace)
 - assignee = issuable.assignee
 - issuable_type = issuable.class.table_name
-- base_url_args = [project.namespace.becomes(Namespace), project, issuable_type]
+- base_url_args = [namespace, project]
+- issuable_type_args = base_url_args + [issuable_type]
+- issuable_url_args = base_url_args + [issuable]
 - can_update = can?(current_user, :"update_#{issuable.to_ability_name}", issuable)
 
-%li{ id: dom_id(issuable, 'sortable'),  class: "issuable-row #{'is-disabled' unless can_update}", 'data-iid' => issuable.iid, 'data-id' => issuable.id, 'data-url' => polymorphic_path(issuable) }
+%li{ id: dom_id(issuable, 'sortable'),  class: "issuable-row #{'is-disabled' unless can_update}", 'data-iid' => issuable.iid, 'data-id' => issuable.id, 'data-url' => polymorphic_path(issuable_url_args) }
   %span
     - if show_project_name
       %strong #{project.name} ·
@@ -13,17 +16,17 @@
       %strong #{project.name_with_namespace} ·
     - if issuable.is_a?(Issue)
       = confidential_icon(issuable)
-    = link_to_gfm issuable.title, [project.namespace.becomes(Namespace), project, issuable], title: issuable.title
+    = link_to_gfm issuable.title, issuable_url_args, title: issuable.title
   .issuable-detail
     = link_to [project.namespace.becomes(Namespace), project, issuable] do
       %span.issuable-number= issuable.to_reference
 
     - issuable.labels.each do |label|
-      = link_to polymorphic_path(base_url_args, { milestone_title: @milestone.title, label_name: label.title, state: 'all' }) do
+      = link_to polymorphic_path(issuable_type_args, { milestone_title: @milestone.title, label_name: label.title, state: 'all' }) do
         - render_colored_label(label)
 
     %span.assignee-icon
       - if assignee
-        = link_to polymorphic_path(base_url_args, { milestone_title: @milestone.title, assignee_id: issuable.assignee_id, state: 'all' }),
+        = link_to polymorphic_path(issuable_type_args, { milestone_title: @milestone.title, assignee_id: issuable.assignee_id, state: 'all' }),
                   class: 'has-tooltip', title: "Assigned to #{assignee.name}", data: { container: 'body' } do
           - image_tag(avatar_icon(issuable.assignee, 16), class: "avatar s16", alt: '')

changelogs/unreleased/sh-optimize-milestone-polymorphic-url.yml

@@ -0,0 +1,4 @@
+---
+title: Eliminate N+1 queries in loading namespaces for every issuable in milestones
+merge_request:
+author:

spec/controllers/projects/milestones_controller_spec.rb

@@ -5,6 +5,7 @@ describe Projects::MilestonesController do
   let(:user)    { create(:user) }
   let(:milestone) { create(:milestone, project: project) }
   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) }
 
   before do
@@ -13,6 +14,20 @@ describe Projects::MilestonesController do
     controller.instance_variable_set(:@project, project)
   end
 
+  describe "#show" do
+    render_views
+
+    def view_milestone
+      get :show, namespace_id: project.namespace.id, project_id: project.id, id: milestone.iid
+    end
+
+    it 'shows milestone page' do
+      view_milestone
+
+      expect(response).to have_http_status(200)
+    end
+  end
+
   describe "#destroy" do
     it "removes milestone" do
       expect(issue.milestone_id).to eq(milestone.id)