Merge branch 'refactoring/issues_filter' into 'master'

Refactoring: Issues/MR filtering logic

Move all issues, mr filtering logic into FilteringService

app/contexts/filter_context.rb

@@ -1,58 +0,0 @@
-class FilterContext
-  attr_accessor :klass, :current_user, :params
-
-  def initialize(klass, current_user, params)
-    @klass = klass
-    @current_user = current_user
-    @params = params
-  end
-
-  def execute
-    items = by_scope
-    items = by_state(items)
-    items = by_project(items)
-    items = by_search(items)
-  end
-
-  private
-
-  def by_scope
-    table_name = klass.table_name
-
-    case params[:scope]
-    when 'authored' then
-      current_user.send(table_name)
-    when 'all' then
-      klass.of_projects(current_user.authorized_projects.pluck(:id))
-    else
-      current_user.send("assigned_#{table_name}")
-    end
-  end
-
-  def by_state(items)
-    case params[:status]
-    when 'closed'
-      items.closed
-    when 'all'
-      items
-    else
-      items.opened
-    end
-  end
-
-  def by_project(items)
-    if params[:project_id].present?
-      items = items.of_projects(params[:project_id])
-    end
-
-    items
-  end
-
-  def by_search(items)
-    if params[:search].present?
-      items = items.search(params[:search])
-    end
-
-    items
-  end
-end

app/contexts/issues/list_context.rb

@@ -1,39 +0,0 @@
-module Issues
-  class ListContext < BaseContext
-    attr_accessor :issues
-
-    def execute
-      @issues = @project.issues
-
-      @issues = case params[:state]
-                when 'all' then @issues
-                when 'closed' then @issues.closed
-                else @issues.opened
-                end
-
-      @issues = case params[:scope]
-                when 'assigned-to-me' then @issues.assigned_to(current_user)
-                when 'created-by-me' then @issues.authored(current_user)
-                else @issues
-                end
-
-      @issues = @issues.tagged_with(params[:label_name]) if params[:label_name].present?
-      @issues = @issues.includes(:author, :project)
-
-      # Filter by specific assignee_id (or lack thereof)?
-      if params[:assignee_id].present?
-        @issues = @issues.where(assignee_id: (params[:assignee_id] == '0' ? nil : params[:assignee_id]))
-      end
-
-      # Filter by specific milestone_id (or lack thereof)?
-      if params[:milestone_id].present?
-        @issues = @issues.where(milestone_id: (params[:milestone_id] == '0' ? nil : params[:milestone_id]))
-      end
-
-      # Sort by :sort param
-      @issues = @issues.sort(params[:sort])
-
-      @issues
-    end
-  end
-end

app/contexts/merge_requests_load_context.rb

@@ -1,38 +0,0 @@
-# Build collection of Merge Requests
-# based on filtering passed via params for @project
-class MergeRequestsLoadContext < BaseContext
-  def execute
-    merge_requests = @project.merge_requests
-
-    merge_requests = case params[:state]
-                     when 'all' then merge_requests
-                     when 'closed' then merge_requests.closed
-                     else merge_requests.opened
-                     end
-
-    merge_requests = case params[:scope]
-                     when 'assigned-to-me' then merge_requests.assigned_to(current_user)
-                     when 'created-by-me' then merge_requests.authored(current_user)
-                     else merge_requests
-                     end
-
-
-    merge_requests = merge_requests.page(params[:page]).per(20)
-    merge_requests = merge_requests.includes(:author, :source_project, :target_project).order("created_at desc")
-
-    # Filter by specific assignee_id (or lack thereof)?
-    if params[:assignee_id].present?
-      merge_requests = merge_requests.where(assignee_id: (params[:assignee_id] == '0' ? nil : params[:assignee_id]))
-    end
-
-    # Filter by specific milestone_id (or lack thereof)?
-    if params[:milestone_id].present?
-      merge_requests = merge_requests.where(milestone_id: (params[:milestone_id] == '0' ? nil : params[:milestone_id]))
-    end
-
-    # Sort by :sort param
-    merge_requests = merge_requests.sort(params[:sort])
-
-    merge_requests
-  end
-end

app/controllers/dashboard_controller.rb

@@ -3,6 +3,8 @@ class DashboardController < ApplicationController
 
   before_filter :load_projects, except: [:projects]
   before_filter :event_filter, only: :show
+  before_filter :default_filter, only: [:issues, :merge_requests]
+
 
   def show
     # Fetch only 30 projects.
@@ -51,12 +53,12 @@ class DashboardController < ApplicationController
   end
 
   def merge_requests
-    @merge_requests = FilterContext.new(MergeRequest, current_user, params).execute
+    @merge_requests = FilteringService.new.execute(MergeRequest, current_user, params)
     @merge_requests = @merge_requests.recent.page(params[:page]).per(20)
   end
 
   def issues
-    @issues = FilterContext.new(Issue, current_user, params).execute
+    @issues = FilteringService.new.execute(Issue, current_user, params)
     @issues = @issues.recent.page(params[:page]).per(20)
     @issues = @issues.includes(:author, :project)
 
@@ -71,4 +73,9 @@ class DashboardController < ApplicationController
   def load_projects
     @projects = current_user.authorized_projects.sorted_by_activity.non_archived
   end
+
+  def default_filter
+    params[:scope] = 'assigned-to-me' if params[:scope].blank?
+    params[:state] = 'opened' if params[:state].blank?
+  end
 end

app/controllers/groups_controller.rb

@@ -10,6 +10,8 @@ class GroupsController < ApplicationController
   # Load group projects
   before_filter :projects, except: [:new, :create]
 
+  before_filter :default_filter, only: [:issues, :merge_requests]
+
   layout :determine_layout
 
   before_filter :set_title, only: [:new, :create]
@@ -43,18 +45,14 @@ class GroupsController < ApplicationController
     end
   end
 
-  # Get authored or assigned open merge requests
   def merge_requests
-    @merge_requests = FilterContext.new(MergeRequest, current_user, params).execute
-    @merge_requests = @merge_requests.of_group(@group)
-    @merge_requests = @merge_requests.recent.page(params[:page]).per(20)
+    @merge_requests = FilteringService.new.execute(MergeRequest, current_user, params)
+    @merge_requests = @merge_requests.page(params[:page]).per(20)
   end
 
-  # Get only assigned issues
   def issues
-    @issues = FilterContext.new(Issue, current_user, params).execute
-    @issues = @issues.of_group(@group)
-    @issues = @issues.recent.page(params[:page]).per(20)
+    @issues = FilteringService.new.execute(Issue, current_user, params)
+    @issues = @issues.page(params[:page]).per(20)
     @issues = @issues.includes(:author, :project)
 
     respond_to do |format|
@@ -130,4 +128,10 @@ class GroupsController < ApplicationController
       'group'
     end
   end
+
+  def default_filter
+    params[:scope] = 'assigned-to-me' if params[:scope].blank?
+    params[:state] = 'opened' if params[:state].blank?
+    params[:group_id] = @group.id
+  end
 end

app/controllers/projects/issues_controller.rb

@@ -116,7 +116,10 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def issues_filtered
-    @issues = Issues::ListContext.new(project, current_user, params).execute
+    params[:scope] = 'all' if params[:scope].blank?
+    params[:state] = 'opened' if params[:state].blank?
+    params[:project_id] = @project.id
+    @issues = FilteringService.new.execute(Issue, current_user, params)
   end
 
   # Since iids are implemented only in 6.1

app/controllers/projects/merge_requests_controller.rb

@@ -17,9 +17,15 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   before_filter :authorize_modify_merge_request!, only: [:close, :edit, :update, :sort]
 
   def index
-    sort_param = params[:sort] || 'newest'
-    @sort = sort_param.humanize unless sort_param.empty?
-    @merge_requests = MergeRequestsLoadContext.new(project, current_user, params).execute
+    params[:sort] ||= 'newest'
+    params[:scope] = 'all' if params[:scope].blank?
+    params[:state] = 'opened' if params[:state].blank?
+    params[:project_id] = @project.id
+
+    @merge_requests = FilteringService.new.execute(MergeRequest, current_user, params)
+    @merge_requests = @merge_requests.page(params[:page]).per(20)
+
+    @sort = params[:sort].humanize
     assignee_id, milestone_id = params[:assignee_id], params[:milestone_id]
     @assignee = @project.team.find(assignee_id) if assignee_id.present? && !assignee_id.to_i.zero?
     @milestone = @project.milestones.find(milestone_id) if milestone_id.present? && !milestone_id.to_i.zero?

app/helpers/dashboard_helper.rb

@@ -1,7 +1,7 @@
 module DashboardHelper
   def filter_path(entity, options={})
     exist_opts = {
-      status: params[:status],
+      state: params[:state],
       scope: params[:scope],
       project_id: params[:project_id],
     }

app/services/filtering_service.rb

@@ -0,0 +1,123 @@
+# FilteringService class
+#
+# Used to filter Issues and MergeRequests collections by set of params
+#
+# Arguments:
+#   klass - actual class like Issue or MergeRequest
+#   current_user - which user use
+#   params:
+#     scope: 'created-by-me' or 'assigned-to-me' or 'all'
+#     state: 'open' or 'closed' or 'all'
+#     group_id: integer
+#     project_id: integer
+#     milestone_id: integer
+#     assignee_id: integer
+#     search: string
+#     label_name: string
+#     sort: string
+#
+class FilteringService
+  attr_accessor :klass, :current_user, :params
+
+  def execute(klass, current_user, params)
+    @klass = klass
+    @current_user = current_user
+    @params = params
+
+    items = by_scope
+    items = by_state(items)
+    items = by_group(items)
+    items = by_project(items)
+    items = by_search(items)
+    items = by_milestone(items)
+    items = by_assignee(items)
+    items = by_label(items)
+    items = sort(items)
+  end
+
+  private
+
+  def by_scope
+    table_name = klass.table_name
+
+    case params[:scope]
+    when 'created-by-me', 'authored' then
+      current_user.send(table_name)
+    when 'all' then
+      if current_user
+        klass.of_projects(current_user.authorized_projects.pluck(:id))
+      else
+        klass.of_projects(Project.public_only)
+      end
+    when 'assigned-to-me' then
+      current_user.send("assigned_#{table_name}")
+    else
+      raise 'You must specify default scope'
+    end
+  end
+
+  def by_state(items)
+    case params[:state]
+    when 'closed'
+      items.closed
+    when 'all'
+      items
+    when 'opened'
+      items.opened
+    else
+      raise 'You must specify default state'
+    end
+  end
+
+  def by_group(items)
+    if params[:group_id].present?
+      items = items.of_group(Group.find(params[:group_id]))
+    end
+
+    items
+  end
+
+  def by_project(items)
+    if params[:project_id].present?
+      items = items.of_projects(params[:project_id])
+    end
+
+    items
+  end
+
+  def by_search(items)
+    if params[:search].present?
+      items = items.search(params[:search])
+    end
+
+    items
+  end
+
+  def sort(items)
+    items.sort(params[:sort])
+  end
+
+  def by_milestone(items)
+    if params[:milestone_id].present?
+      items = items.where(milestone_id: (params[:milestone_id] == '0' ? nil : params[:milestone_id]))
+    end
+
+    items
+  end
+
+  def by_assignee(items)
+    if params[:assignee_id].present?
+      items = items.where(assignee_id: (params[:assignee_id] == '0' ? nil : params[:assignee_id]))
+    end
+
+    items
+  end
+
+  def by_label(items)
+    if params[:label_name].present?
+      items = items.tagged_with(params[:label_name])
+    end
+
+    items
+  end
+end

app/views/shared/_filter.html.haml

@@ -2,8 +2,8 @@
   = form_tag filter_path(entity), method: 'get' do
     %fieldset.scope-filter
       %ul.nav.nav-pills.nav-stacked
-        %li{class: ("active" if params[:scope].blank?)}
-          = link_to filter_path(entity, scope: nil) do
+        %li{class: ("active" if params[:scope] == 'assigned-to-me')}
+          = link_to filter_path(entity, scope: 'assigned-to-me') do
             Assigned to me
         %li{class: ("active" if params[:scope] == 'authored')}
           = link_to filter_path(entity, scope: 'authored') do
@@ -15,14 +15,14 @@
     %fieldset.status-filter
       %legend State
       %ul.nav.nav-pills
-        %li{class: ("active" if params[:status].blank?)}
-          = link_to filter_path(entity, status: nil) do
+        %li{class: ("active" if params[:state] == 'opened')}
+          = link_to filter_path(entity, state: 'opened') do
             Open
-        %li{class: ("active" if params[:status] == 'closed')}
-          = link_to filter_path(entity, status: 'closed') do
+        %li{class: ("active" if params[:state] == 'closed')}
+          = link_to filter_path(entity, state: 'closed') do
             Closed
-        %li{class: ("active" if params[:status] == 'all')}
-          = link_to filter_path(entity, status: 'all') do
+        %li{class: ("active" if params[:state] == 'all')}
+          = link_to filter_path(entity, state: 'all') do
             All
 
     %fieldset
@@ -36,8 +36,8 @@
                 %small.pull-right= entities_per_project(project, entity)
 
     %fieldset
-      - if params[:status].present? || params[:project_id].present?
-        = link_to filter_path(entity, status: nil, project_id: nil), class: 'pull-right cgray' do
+      - if params[:state].present? || params[:project_id].present?
+        = link_to filter_path(entity, state: nil, project_id: nil), class: 'pull-right cgray' do
           %i.icon-remove
           %strong Clear filter
 

app/views/shared/_project_filter.html.haml

@@ -3,8 +3,8 @@
     - if current_user
       %fieldset
         %ul.nav.nav-pills.nav-stacked
-          %li{class: ("active" if params[:scope].blank?)}
-            = link_to project_filter_path(scope: nil) do
+          %li{class: ("active" if params[:scope] == 'all')}
+            = link_to project_filter_path(scope: 'all') do
               Everyone's
           %li{class: ("active" if params[:scope] == 'assigned-to-me')}
             = link_to project_filter_path(scope: 'assigned-to-me') do
@@ -16,8 +16,8 @@
     %fieldset
       %legend State
       %ul.nav.nav-pills
-        %li{class: ("active" if params[:state].blank?)}
-          = link_to project_filter_path(state: nil) do
+        %li{class: ("active" if params[:state] == 'opened')}
+          = link_to project_filter_path(state: 'opened') do
             Open
         %li{class: ("active" if params[:state] == 'closed')}
           = link_to project_filter_path(state: 'closed') do

spec/contexts/issues/list_context_spec.rb

@@ -1,73 +0,0 @@
-require 'spec_helper'
-
-describe Issues::ListContext do
-
-  let(:user) { create(:user) }
-  let(:project) { create(:project, creator: user) }
-
-  titles = ['foo','bar','baz']
-  titles.each_with_index do |title, index|
-    let!(title.to_sym) { create(:issue, title: title, project: project, created_at: Time.now - (index * 60)) }
-  end
-
-  describe 'sorting' do
-    it 'sorts by newest' do
-      params = {sort: 'newest'}
-
-      issues = Issues::ListContext.new(project, user, params).execute
-      issues.first.should eq foo
-    end
-
-    it 'sorts by oldest' do
-      params = {sort: 'oldest'}
-
-      issues = Issues::ListContext.new(project, user, params).execute
-      issues.first.should eq baz
-    end
-
-    it 'sorts by recently updated' do
-      params = {sort: 'recently_updated'}
-      baz.updated_at = Time.now + 10
-      baz.save
-
-      issues = Issues::ListContext.new(project, user, params).execute
-      issues.first.should eq baz
-    end
-
-    it 'sorts by least recently updated' do
-      params = {sort: 'last_updated'}
-      bar.updated_at = Time.now - 10
-      bar.save
-
-      issues = Issues::ListContext.new(project, user, params).execute
-      issues.first.should eq bar
-    end
-
-    describe 'sorting by milestone' do
-      let(:newer_due_milestone) { create(:milestone, due_date: '2013-12-11') }
-      let(:later_due_milestone) { create(:milestone, due_date: '2013-12-12') }
-
-      before :each do
-        foo.milestone = newer_due_milestone
-        foo.save
-        bar.milestone = later_due_milestone
-        bar.save
-      end
-
-      it 'sorts by most recently due milestone' do
-        params = {sort: 'milestone_due_soon'}
-
-        issues = Issues::ListContext.new(project, user, params).execute
-        issues.first.should eq foo
-
-      end
-
-      it 'sorts by least recently due milestone' do
-        params = {sort: 'milestone_due_later'}
-
-        issues = Issues::ListContext.new(project, user, params).execute
-        issues.first.should eq bar
-      end
-    end
-  end
-end

spec/services/filtering_service_spec.rb

@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe FilterContext do
+describe FilteringService do
   let(:user) { create :user }
   let(:user2) { create :user }
   let(:project1) { create(:project) }
@@ -25,14 +25,14 @@ describe FilterContext do
     end
 
     it 'should filter by scope' do
-      params = { scope: 'authored' }
-      merge_requests = FilterContext.new(MergeRequest, user, params).execute
+      params = { scope: 'authored', state: 'opened' }
+      merge_requests = FilteringService.new.execute(MergeRequest, user, params)
       merge_requests.size.should == 3
     end
 
     it 'should filter by project' do
-      params = { project_id: project1.id, scope: 'authored' }
-      merge_requests = FilterContext.new(MergeRequest, user, params).execute
+      params = { project_id: project1.id, scope: 'authored', state: 'opened' }
+      merge_requests = FilteringService.new.execute(MergeRequest, user, params)
       merge_requests.size.should == 1
     end
   end
@@ -45,20 +45,20 @@ describe FilterContext do
     end
 
     it 'should filter by all' do
-      params = { scope: "all" }
-      issues = FilterContext.new(Issue, user, params).execute
+      params = { scope: "all", state: 'opened' }
+      issues = FilteringService.new.execute(Issue, user, params)
       issues.size.should == 3
     end
 
     it 'should filter by assignee' do
-      params = {}
-      issues = FilterContext.new(Issue, user, params).execute
+      params = { scope: "assigned-to-me", state: 'opened' }
+      issues = FilteringService.new.execute(Issue, user, params)
       issues.size.should == 2
     end
 
     it 'should filter by project' do
-      params = { project_id: project1.id }
-      issues = FilterContext.new(Issue, user, params).execute
+      params = { scope: "assigned-to-me", state: 'opened', project_id: project1.id }
+      issues = FilteringService.new.execute(Issue, user, params)
       issues.size.should == 1
     end
   end