Merge branch 'browse-milestone-issues' into 'master'

Fix milestone "Browse Issues" button.

After refactoring around issue/merge requests filters, the button stopped working :(

See merge request !699

CHANGELOG

@@ -6,6 +6,8 @@ v 7.12.0 (unreleased)
   - Fix Zen Mode not closing with ESC key (Stan Hu)
   - Allow HipChat API version to be blank and default to v2 (Stan Hu)
   - Add file attachment support in Milestone description (Stan Hu)
+  - Fix milestone "Browse Issues" button.
+  - Set milestone on new issue when creating issue from index with milestone filter active.
   - Add web hook support for note events (Stan Hu)
   - Disable "New Issue" and "New Merge Request" buttons when features are disabled in project settings (Stan Hu)
   - Remove Rack Attack monkey patches and bump to version 4.3.0 (Stan Hu)

app/controllers/application_controller.rb

@@ -289,14 +289,14 @@ class ApplicationController < ActionController::Base
 
   def get_issues_collection
     set_filters_params
-    issues = IssuesFinder.new.execute(current_user, @filter_params)
-    issues
+    @issuable_finder = IssuesFinder.new(current_user, @filter_params)
+    @issuable_finder.execute
   end
 
   def get_merge_requests_collection
     set_filters_params
-    merge_requests = MergeRequestsFinder.new.execute(current_user, @filter_params)
-    merge_requests
+    @issuable_finder = MergeRequestsFinder.new(current_user, @filter_params)
+    @issuable_finder.execute
   end
 
   def github_import_enabled?

app/finders/README.md

@@ -16,7 +16,7 @@ issues = project.issues_for_user_filtered_by(user, params)
 Better use this:
 
 ```ruby
-issues = IssuesFinder.new.execute(project, user, filter)
+issues = IssuesFinder.new(project, user, filter).execute
 ```
 
 It will help keep models thiner.

app/finders/issuable_finder.rb

@@ -23,10 +23,12 @@ class IssuableFinder
 
   attr_accessor :current_user, :params
 
-  def execute(current_user, params)
+  def initialize(current_user, params)
     @current_user = current_user
     @params = params
+  end
 
+  def execute
     items = init_collection
     items = by_scope(items)
     items = by_state(items)
@@ -40,6 +42,77 @@ class IssuableFinder
     items = sort(items)
   end
 
+  def group
+    return @group if defined?(@group)
+
+    @group = 
+      if params[:group_id].present?
+        Group.find(params[:group_id])
+      else 
+        nil
+      end
+  end
+
+  def project
+    return @project if defined?(@project)
+
+    @project = 
+      if params[:project_id].present?
+        Project.find(params[:project_id])
+      else 
+        nil
+      end
+  end
+
+  def search
+    params[:search].presence
+  end
+
+  def milestones?
+    params[:milestone_title].present?
+  end
+
+  def milestones
+    return @milestones if defined?(@milestones)
+
+    @milestones =
+      if milestones? && params[:milestone_title] != NONE 
+        Milestone.where(title: params[:milestone_title])
+      else
+        nil
+      end
+  end
+
+  def assignee?
+    params[:assignee_id].present?
+  end
+
+  def assignee
+    return @assignee if defined?(@assignee)
+
+    @assignee = 
+      if assignee? && params[:assignee_id] != NONE
+        User.find(params[:assignee_id])
+      else
+        nil
+      end
+  end
+
+  def author?
+    params[:author_id].present?
+  end
+
+  def author
+    return @author if defined?(@author)
+
+    @author = 
+      if author? && params[:author_id] != NONE
+        User.find(params[:author_id])
+      else
+        nil
+      end
+  end
+
   private
 
   def init_collection
@@ -89,25 +162,19 @@ class IssuableFinder
   end
 
   def by_group(items)
-    if params[:group_id].present?
-      items = items.of_group(Group.find(params[:group_id]))
-    end
+    items = items.of_group(group) if group
 
     items
   end
 
   def by_project(items)
-    if params[:project_id].present?
-      items = items.of_projects(params[:project_id])
-    end
+    items = items.of_projects(project.id) if project
 
     items
   end
 
   def by_search(items)
-    if params[:search].present?
-      items = items.search(params[:search])
-    end
+    items = items.search(search) if search
 
     items
   end
@@ -117,25 +184,24 @@ class IssuableFinder
   end
 
   def by_milestone(items)
-    if params[:milestone_title].present?
-      milestone_ids = (params[:milestone_title] == NONE ? nil : Milestone.where(title: params[:milestone_title]).pluck(:id))
-      items = items.where(milestone_id: milestone_ids)
+    if milestones?
+      items = items.where(milestone_id: milestones.try(:pluck, :id))
     end
 
     items
   end
 
   def by_assignee(items)
-    if params[:assignee_id].present?
-      items = items.where(assignee_id: (params[:assignee_id] == NONE ? nil : params[:assignee_id]))
+    if assignee?
+      items = items.where(assignee_id: assignee.try(:id))
     end
 
     items
   end
 
   def by_author(items)
-    if params[:author_id].present?
-      items = items.where(author_id: (params[:author_id] == NONE ? nil : params[:author_id]))
+    if author?
+      items = items.where(author_id: author.try(:id))
     end
 
     items
@@ -155,10 +221,6 @@ class IssuableFinder
     items
   end
 
-  def project
-    Project.where(id: params[:project_id]).first if params[:project_id].present?
-  end
-
   def current_user_related?
     params[:scope] == 'created-by-me' || params[:scope] == 'authored' || params[:scope] == 'assigned-to-me'
   end

app/views/dashboard/milestones/_milestone.html.haml

@@ -3,10 +3,10 @@
     = link_to_gfm truncate(milestone.title, length: 100), dashboard_milestone_path(milestone.safe_title, title: milestone.title)
   .row
     .col-sm-6
-      = link_to dashboard_milestone_path(milestone.safe_title, title: milestone.title) do
+      = link_to issues_dashboard_path(milestone_title: milestone.title) do
         = pluralize milestone.issue_count, 'Issue'
        
-      = link_to dashboard_milestone_path(milestone.safe_title, title: milestone.title) do
+      = link_to merge_requests_dashboard_path(milestone_title: milestone.title) do
         = pluralize milestone.merge_requests_count, 'Merge Request'
        
       %span.light #{milestone.percent_complete}% complete

app/views/dashboard/milestones/show.html.haml

@@ -56,6 +56,9 @@
       Participants
       %span.badge= @dashboard_milestone.participants.count
 
+  .pull-right
+    = link_to 'Browse Issues', issues_dashboard_path(milestone_title: @dashboard_milestone.title), class: "btn  edit-milestone-link btn-grouped"
+
 .tab-content
   .tab-pane.active#tab-issues
     .row

app/views/groups/milestones/_milestone.html.haml

@@ -9,10 +9,10 @@
     = link_to_gfm truncate(milestone.title, length: 100), group_milestone_path(@group, milestone.safe_title, title: milestone.title)
   .row
     .col-sm-6
-      = link_to group_milestone_path(@group, milestone.safe_title, title: milestone.title) do
+      = link_to issues_group_path(@group, milestone_title: milestone.title) do
         = pluralize milestone.issue_count, 'Issue'
        
-      = link_to group_milestone_path(@group, milestone.safe_title, title: milestone.title) do
+      = link_to merge_requests_group_path(@group, milestone_title: milestone.title) do
         = pluralize milestone.merge_requests_count, 'Merge Request'
        
       %span.light #{milestone.percent_complete}% complete

app/views/groups/milestones/show.html.haml

@@ -62,6 +62,9 @@
       Participants
       %span.badge= @group_milestone.participants.count
 
+  .pull-right
+    = link_to 'Browse Issues', issues_group_path(@group, milestone_title: @group_milestone.title), class: "btn  edit-milestone-link btn-grouped"
+
 .tab-content
   .tab-pane.active#tab-issues
     .row

app/views/projects/issues/index.html.haml

@@ -14,7 +14,7 @@
       = render 'shared/issuable_search_form', path: namespace_project_issues_path(@project.namespace, @project)
 
     - if can? current_user, :write_issue, @project
-      = link_to new_namespace_project_issue_path(@project.namespace, @project, issue: { assignee_id: params[:assignee_id], milestone_id: params[:milestone_id]}), class: "btn btn-new pull-left", title: "New Issue", id: "new_issue_link" do
+      = link_to new_namespace_project_issue_path(@project.namespace, @project, issue: { assignee_id: @issuable_finder.assignee.try(:id), milestone_id: @issuable_finder.milestones.try(:first).try(:id) }), class: "btn btn-new pull-left", title: "New Issue", id: "new_issue_link" do
         %i.fa.fa-plus
         New Issue
 

app/views/projects/milestones/_milestone.html.haml

@@ -13,10 +13,10 @@
       = milestone.expires_at
   .row
     .col-sm-6
-      = link_to namespace_project_issues_path(milestone.project.namespace, milestone.project, milestone_id: milestone.id) do
+      = link_to namespace_project_issues_path(milestone.project.namespace, milestone.project, milestone_title: milestone.title) do
         = pluralize milestone.issues.count, 'Issue'
        
-      = link_to namespace_project_merge_requests_path(milestone.project.namespace, milestone.project, milestone_id: milestone.id) do
+      = link_to namespace_project_merge_requests_path(milestone.project.namespace, milestone.project, milestone_title: milestone.title) do
         = pluralize milestone.merge_requests.count, 'Merge Request'
        
       %span.light #{milestone.percent_complete}% complete

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

@@ -67,7 +67,7 @@
         %i.fa.fa-plus
         New Issue
     - if can?(current_user, :read_issue, @project)
-      = link_to 'Browse Issues', namespace_project_issues_path(@milestone.project.namespace, @milestone.project, milestone_id: @milestone.id), class: "btn  edit-milestone-link btn-grouped"
+      = link_to 'Browse Issues', namespace_project_issues_path(@milestone.project.namespace, @milestone.project, milestone_title: @milestone.title), class: "btn  edit-milestone-link btn-grouped"
 
 .tab-content
   .tab-pane.active#tab-issues

features/steps/groups.rb

@@ -203,8 +203,8 @@ class Spinach::Features::Groups < Spinach::FeatureSteps
   step 'I should see group milestones index page with milestones' do
     page.should have_content('Version 7.2')
     page.should have_content('GL-113')
-    page.should have_link('2 Issues', href: group_milestone_path("owned", "version-7-2", title: "Version 7.2"))
-    page.should have_link('3 Merge Requests', href: group_milestone_path("owned", "gl-113", title: "GL-113"))
+    page.should have_link('2 Issues', href: issues_group_path("owned", milestone_title: "Version 7.2"))
+    page.should have_link('3 Merge Requests', href: merge_requests_group_path("owned", milestone_title: "GL-113"))
   end
 
   step 'I click on one group milestone' do

spec/finders/issues_finder_spec.rb

@@ -26,37 +26,37 @@ describe IssuesFinder do
     context 'scope: all' do
       it 'should filter by all' do
         params = { scope: "all", state: 'opened' }
-        issues = IssuesFinder.new.execute(user, params)
+        issues = IssuesFinder.new(user, params).execute
         expect(issues.size).to eq(3)
       end
 
       it 'should filter by assignee id' do
         params = { scope: "all", assignee_id: user.id, state: 'opened' }
-        issues = IssuesFinder.new.execute(user, params)
+        issues = IssuesFinder.new(user, params).execute
         expect(issues.size).to eq(2)
       end
 
       it 'should filter by author id' do
         params = { scope: "all", author_id: user2.id, state: 'opened' }
-        issues = IssuesFinder.new.execute(user, params)
+        issues = IssuesFinder.new(user, params).execute
         expect(issues).to eq([issue3])
       end
 
       it 'should filter by milestone id' do
         params = { scope: "all", milestone_title: milestone.title, state: 'opened' }
-        issues = IssuesFinder.new.execute(user, params)
+        issues = IssuesFinder.new(user, params).execute
         expect(issues).to eq([issue1])
       end
 
       it 'should be empty for unauthorized user' do
         params = { scope: "all", state: 'opened' }
-        issues = IssuesFinder.new.execute(nil, params)
+        issues = IssuesFinder.new(nil, params).execute
         expect(issues.size).to be_zero
       end
 
       it 'should not include unauthorized issues' do
         params = { scope: "all", state: 'opened' }
-        issues = IssuesFinder.new.execute(user2, params)
+        issues = IssuesFinder.new(user2, params).execute
         expect(issues.size).to eq(2)
         expect(issues).not_to include(issue1)
         expect(issues).to include(issue2)
@@ -67,13 +67,13 @@ describe IssuesFinder do
     context 'personal scope' do
       it 'should filter by assignee' do
         params = { scope: "assigned-to-me", state: 'opened' }
-        issues = IssuesFinder.new.execute(user, params)
+        issues = IssuesFinder.new(user, params).execute
         expect(issues.size).to eq(2)
       end
 
       it 'should filter by project' do
         params = { scope: "assigned-to-me", state: 'opened', project_id: project1.id }
-        issues = IssuesFinder.new.execute(user, params)
+        issues = IssuesFinder.new(user, params).execute
         expect(issues.size).to eq(1)
       end
     end

spec/finders/merge_requests_finder_spec.rb

@@ -20,13 +20,13 @@ describe MergeRequestsFinder do
   describe "#execute" do
     it 'should filter by scope' do
       params = { scope: 'authored', state: 'opened' }
-      merge_requests = MergeRequestsFinder.new.execute(user, params)
+      merge_requests = MergeRequestsFinder.new(user, params).execute
       expect(merge_requests.size).to eq(2)
     end
 
     it 'should filter by project' do
       params = { project_id: project1.id, scope: 'authored', state: 'opened' }
-      merge_requests = MergeRequestsFinder.new.execute(user, params)
+      merge_requests = MergeRequestsFinder.new(user, params).execute
       expect(merge_requests.size).to eq(1)
     end
   end