Commit 84a3225b authored by zs's avatar zs
Browse files

State specific default sort order for issuables

Provide more sensible default sort order for issues and merge requests
based on the following table:

| type           | state  | default sort order |
|----------------|--------|--------------------|
| issues         | open   | last created       |
| issues         | closed | last updated       |
| issues         | all    | last created       |
| merge requests | open   | last created       |
| merge requests | merged | last updated       |
| merge requests | closed | last updated       |
| merge requests | all    | last created       |
parent 242f8377
......@@ -32,6 +32,7 @@ v 8.11.0 (unreleased)
- Make error pages responsive (Takuya Noguchi)
- Change requests_profiles resource constraint to catch virtually any file
- Reduce number of queries made for merge_requests/:id/diffs
- Sensible state specific default sort order for issues and merge requests !5453 (tomb0y)
v 8.10.3 (unreleased)
......
......@@ -243,42 +243,6 @@ def require_email
end
end
def set_filters_params
set_default_sort
params[:scope] = 'all' if params[:scope].blank?
params[:state] = 'opened' if params[:state].blank?
@sort = params[:sort]
@filter_params = params.dup
if @project
@filter_params[:project_id] = @project.id
elsif @group
@filter_params[:group_id] = @group.id
else
# TODO: this filter ignore issues/mr created in public or
# internal repos where you are not a member. Enable this filter
# or improve current implementation to filter only issues you
# created or assigned or mentioned
# @filter_params[:authorized_only] = true
end
@filter_params
end
def get_issues_collection
set_filters_params
@issuable_finder = IssuesFinder.new(current_user, @filter_params)
@issuable_finder.execute
end
def get_merge_requests_collection
set_filters_params
@issuable_finder = MergeRequestsFinder.new(current_user, @filter_params)
@issuable_finder.execute
end
def import_sources_enabled?
!current_application_settings.import_sources.empty?
end
......@@ -363,24 +327,4 @@ def redirect_to_home_page_url?
def u2f_app_id
request.base_url
end
private
def set_default_sort
key = if is_a_listing_page_for?('issues') || is_a_listing_page_for?('merge_requests')
'issuable_sort'
end
cookies[key] = params[:sort] if key && params[:sort].present?
params[:sort] = cookies[key] if key
params[:sort] ||= 'id_desc'
end
def is_a_listing_page_for?(page_type)
controller_name, action_name = params.values_at(:controller, :action)
(controller_name == "projects/#{page_type}" && action_name == 'index') ||
(controller_name == 'groups' && action_name == page_type) ||
(controller_name == 'dashboard' && action_name == page_type)
end
end
module IssuableCollections
extend ActiveSupport::Concern
include SortingHelper
included do
helper_method :issues_finder
helper_method :merge_requests_finder
end
private
def issues_collection
issues_finder.execute
end
def merge_requests_collection
merge_requests_finder.execute
end
def issues_finder
@issues_finder ||= issuable_finder_for(IssuesFinder)
end
def merge_requests_finder
@merge_requests_finder ||= issuable_finder_for(MergeRequestsFinder)
end
def issuable_finder_for(finder_class)
finder_class.new(current_user, filter_params)
end
def filter_params
set_sort_order_from_cookie
set_default_scope
set_default_state
@filter_params = params.dup
@filter_params[:sort] ||= default_sort_order
@sort = @filter_params[:sort]
if @project
@filter_params[:project_id] = @project.id
elsif @group
@filter_params[:group_id] = @group.id
else
# TODO: this filter ignore issues/mr created in public or
# internal repos where you are not a member. Enable this filter
# or improve current implementation to filter only issues you
# created or assigned or mentioned
# @filter_params[:authorized_only] = true
end
@filter_params
end
def set_default_scope
params[:scope] = 'all' if params[:scope].blank?
end
def set_default_state
params[:state] = 'opened' if params[:state].blank?
end
def set_sort_order_from_cookie
key = 'issuable_sort'
cookies[key] = params[:sort] if params[:sort].present?
params[:sort] = cookies[key]
end
def default_sort_order
case params[:state]
when 'opened', 'all' then sort_value_recently_created
when 'merged', 'closed' then sort_value_recently_updated
else sort_value_recently_created
end
end
end
module IssuesAction
extend ActiveSupport::Concern
include IssuableCollections
def issues
@issues = get_issues_collection.non_archived
@issues = @issues.page(params[:page])
@issues = @issues.preload(:author, :project)
@label = issues_finder.labels.first
@label = @issuable_finder.labels.first
@issues = issues_collection
.non_archived
.preload(:author, :project)
.page(params[:page])
respond_to do |format|
format.html
......
module MergeRequestsAction
extend ActiveSupport::Concern
include IssuableCollections
def merge_requests
@merge_requests = get_merge_requests_collection.non_archived
@merge_requests = @merge_requests.page(params[:page])
@merge_requests = @merge_requests.preload(:author, :target_project)
@label = merge_requests_finder.labels.first
@label = @issuable_finder.labels.first
@merge_requests = merge_requests_collection
.non_archived
.preload(:author, :target_project)
.page(params[:page])
end
end
......@@ -3,6 +3,7 @@ class Projects::IssuesController < Projects::ApplicationController
include ToggleSubscriptionAction
include IssuableActions
include ToggleAwardEmoji
include IssuableCollections
before_action :module_enabled
before_action :issue, only: [:edit, :update, :show, :referenced_merge_requests,
......@@ -24,7 +25,7 @@ class Projects::IssuesController < Projects::ApplicationController
def index
terms = params['issue_search']
@issues = get_issues_collection
@issues = issues_collection
if terms.present?
if terms =~ /\A#(\d+)\z/
......
......@@ -5,6 +5,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
include IssuableActions
include NotesHelper
include ToggleAwardEmoji
include IssuableCollections
before_action :module_enabled
before_action :merge_request, only: [
......@@ -29,7 +30,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
def index
terms = params['issue_search']
@merge_requests = get_merge_requests_collection
@merge_requests = merge_requests_collection
if terms.present?
if terms =~ /\A[#!](\d+)\z/
......
......@@ -109,7 +109,7 @@ def milestones
scope.where(title: params[:milestone_title])
else
nil
Milestone.none
end
end
......
......@@ -245,7 +245,6 @@ def page_filter_path(options = {})
milestone_title: params[:milestone_title],
assignee_id: params[:assignee_id],
author_id: params[:author_id],
sort: params[:sort],
issue_search: params[:issue_search],
label_name: params[:label_name]
}
......
......@@ -102,11 +102,11 @@ def sort_value_recently_updated
end
def sort_value_oldest_created
'id_asc'
'created_asc'
end
def sort_value_recently_created
'id_desc'
'created_desc'
end
def sort_value_milestone_soon
......
......@@ -19,7 +19,13 @@
Subscribe
= render 'shared/issuable/search_form', path: namespace_project_issues_path(@project.namespace, @project)
- if can? current_user, :create_issue, @project
= 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", title: "New Issue", id: "new_issue_link" do
= link_to new_namespace_project_issue_path(@project.namespace,
@project,
issue: { assignee_id: issues_finder.assignee.try(:id),
milestone_id: issues_finder.milestones.first.try(:id) }),
class: "btn btn-new",
title: "New Issue",
id: "new_issue_link" do
New Issue
= render 'shared/issuable/filter', type: :issues
......
require 'spec_helper'
describe 'Projects > Issuables > Default sort order', feature: true do
let(:project) { create(:empty_project, :public) }
let(:first_created_issuable) { issuables.order_created_asc.first }
let(:last_created_issuable) { issuables.order_created_desc.first }
let(:first_updated_issuable) { issuables.order_updated_asc.first }
let(:last_updated_issuable) { issuables.order_updated_desc.first }
context 'for merge requests' do
include MergeRequestHelpers
let!(:issuables) do
timestamps = [{ created_at: 3.minutes.ago, updated_at: 20.seconds.ago },
{ created_at: 2.minutes.ago, updated_at: 30.seconds.ago },
{ created_at: 4.minutes.ago, updated_at: 10.seconds.ago }]
timestamps.each_with_index do |ts, i|
create issuable_type, { title: "#{issuable_type}_#{i}",
source_branch: "#{issuable_type}_#{i}",
source_project: project }.merge(ts)
end
MergeRequest.all
end
context 'in the "merge requests" tab', js: true do
let(:issuable_type) { :merge_request }
it 'is "last created"' do
visit_merge_requests project
expect(first_merge_request).to include(last_created_issuable.title)
expect(last_merge_request).to include(first_created_issuable.title)
end
end
context 'in the "merge requests / open" tab', js: true do
let(:issuable_type) { :merge_request }
it 'is "last created"' do
visit_merge_requests_with_state(project, 'open')
expect(selected_sort_order).to eq('last created')
expect(first_merge_request).to include(last_created_issuable.title)
expect(last_merge_request).to include(first_created_issuable.title)
end
end
context 'in the "merge requests / merged" tab', js: true do
let(:issuable_type) { :merged_merge_request }
it 'is "last updated"' do
visit_merge_requests_with_state(project, 'merged')
expect(selected_sort_order).to eq('last updated')
expect(first_merge_request).to include(last_updated_issuable.title)
expect(last_merge_request).to include(first_updated_issuable.title)
end
end
context 'in the "merge requests / closed" tab', js: true do
let(:issuable_type) { :closed_merge_request }
it 'is "last updated"' do
visit_merge_requests_with_state(project, 'closed')
expect(selected_sort_order).to eq('last updated')
expect(first_merge_request).to include(last_updated_issuable.title)
expect(last_merge_request).to include(first_updated_issuable.title)
end
end
context 'in the "merge requests / all" tab', js: true do
let(:issuable_type) { :merge_request }
it 'is "last created"' do
visit_merge_requests_with_state(project, 'all')
expect(selected_sort_order).to eq('last created')
expect(first_merge_request).to include(last_created_issuable.title)
expect(last_merge_request).to include(first_created_issuable.title)
end
end
end
context 'for issues' do
include IssueHelpers
let!(:issuables) do
timestamps = [{ created_at: 3.minutes.ago, updated_at: 20.seconds.ago },
{ created_at: 2.minutes.ago, updated_at: 30.seconds.ago },
{ created_at: 4.minutes.ago, updated_at: 10.seconds.ago }]
timestamps.each_with_index do |ts, i|
create issuable_type, { title: "#{issuable_type}_#{i}",
project: project }.merge(ts)
end
Issue.all
end
context 'in the "issues" tab', js: true do
let(:issuable_type) { :issue }
it 'is "last created"' do
visit_issues project
expect(selected_sort_order).to eq('last created')
expect(first_issue).to include(last_created_issuable.title)
expect(last_issue).to include(first_created_issuable.title)
end
end
context 'in the "issues / open" tab', js: true do
let(:issuable_type) { :issue }
it 'is "last created"' do
visit_issues_with_state(project, 'open')
expect(selected_sort_order).to eq('last created')
expect(first_issue).to include(last_created_issuable.title)
expect(last_issue).to include(first_created_issuable.title)
end
end
context 'in the "issues / closed" tab', js: true do
let(:issuable_type) { :closed_issue }
it 'is "last updated"' do
visit_issues_with_state(project, 'closed')
expect(selected_sort_order).to eq('last updated')
expect(first_issue).to include(last_updated_issuable.title)
expect(last_issue).to include(first_updated_issuable.title)
end
end
context 'in the "issues / all" tab', js: true do
let(:issuable_type) { :issue }
it 'is "last created"' do
visit_issues_with_state(project, 'all')
expect(selected_sort_order).to eq('last created')
expect(first_issue).to include(last_created_issuable.title)
expect(last_issue).to include(first_created_issuable.title)
end
end
end
def selected_sort_order
find('.pull-right .dropdown button').text.downcase
end
def visit_merge_requests_with_state(project, state)
visit_merge_requests project
visit_issuables_with_state state
end
def visit_issues_with_state(project, state)
visit_issues project
visit_issuables_with_state state
end
def visit_issuables_with_state(state)
within('.issues-state-filters') { find("span", text: state.titleize).click }
end
end
require 'spec_helper'
describe 'Issues', feature: true do
include IssueHelpers
include SortingHelper
let(:project) { create(:project) }
......@@ -186,15 +187,15 @@
it 'sorts by newest' do
visit namespace_project_issues_path(project.namespace, project, sort: sort_value_recently_created)
expect(first_issue).to include('baz')
expect(last_issue).to include('foo')
expect(first_issue).to include('foo')
expect(last_issue).to include('baz')
end
it 'sorts by oldest' do
visit namespace_project_issues_path(project.namespace, project, sort: sort_value_oldest_created)
expect(first_issue).to include('foo')
expect(last_issue).to include('baz')
expect(first_issue).to include('baz')
expect(last_issue).to include('foo')
end
it 'sorts by most recently updated' do
......@@ -350,8 +351,8 @@
sort: sort_value_oldest_created,
assignee_id: user2.id)
expect(first_issue).to include('foo')
expect(last_issue).to include('bar')
expect(first_issue).to include('bar')
expect(last_issue).to include('foo')
expect(page).not_to have_content 'baz'
end
end
......@@ -590,14 +591,6 @@
end
end
def first_issue
page.all('ul.issues-list > li').first.text
end
def last_issue
page.all('ul.issues-list > li').last.text
end
def drop_in_dropzone(file_path)
# Generate a fake input selector
page.execute_script <<-JS
......
require 'spec_helper'
describe 'Projects > Merge requests > User lists merge requests', feature: true do
include MergeRequestHelpers
include SortingHelper
let(:project) { create(:project, :public) }
......@@ -23,10 +24,12 @@
milestone: create(:milestone, due_date: '2013-12-12'),
created_at: 2.minutes.ago,
updated_at: 2.minutes.ago)
# lfs in itself is not a great choice for the title if one wants to match the whole body content later on
# just think about the scenario when faker generates 'Chester Runolfsson' as the user's name
create(:merge_request,
title: 'lfs',
title: 'merge_lfs',
source_project: project,
source_branch: 'lfs',
source_branch: 'merge_lfs',
created_at: 3.minutes.ago,
updated_at: 10.seconds.ago)
end
......@@ -35,7 +38,7 @@
visit_merge_requests(project, assignee_id: IssuableFinder::NONE)
expect(current_path).to eq(namespace_project_merge_requests_path(project.namespace, project))
expect(page).to have_content 'lfs'
expect(page).to have_content 'merge_lfs'
expect(page).not_to have_content 'fix'
expect(page).not_to have_content 'markdown'
expect(count_merge_requests).to eq(1)
......@@ -44,7 +47,7 @@
it 'filters on a specific assignee' do
visit_merge_requests(project, assignee_id: user.id)
expect(page).not_to have_content 'lfs'
expect(page).not_to have_content 'merge_lfs'
expect(page).to have_content 'fix'
expect(page).to have_content 'markdown'
expect(count_merge_requests).to eq(2)
......@@ -53,23 +56,23 @@
it 'sorts by newest' do
visit_merge_requests(project, sort: sort_value_recently_created)
expect(first_merge_request).to include('lfs')
expect(last_merge_request).to include('fix')
expect(first_merge_request).to include('fix')
expect(last_merge_request).to include('merge_lfs')
expect(count_merge_requests).to eq(3)
end
it 'sorts by oldest' do
visit_merge_requests(project, sort: sort_value_oldest_created)
expect(first_merge_request).to include('fix')
expect(last_merge_request).to include('lfs')
expect(first_merge_request).to include('merge_lfs')
expect(last_merge_request).to include('fix')
expect(count_merge_requests).to eq(3)
end
it 'sorts by last updated' do
visit_merge_requests(project, sort: sort_value_recently_updated)
expect(first_merge_request).to include('lfs')
expect(first_merge_request).to include('merge_lfs')
expect(count_merge_requests).to eq(3)
end
......@@ -143,18 +146,6 @@
end
end
def visit_merge_requests(project, opts = {})
visit namespace_project_merge_requests_path(project.namespace, project, opts)
end
def first_merge_request
page.all('ul.mr-list > li').first.text
end
def last_merge_request
page.all('ul.mr-list > li').last.text
end
def count_merge_requests
page.all('ul.mr-list > li').count
end
......
module IssueHelpers
def visit_issues(project, opts = {})
visit namespace_project_issues_path project.namespace, project, opts
end
def first_issue
page.all('ul.issues-list > li').first.text
end
def last_issue
page.all('ul.issues-list > li').last.text
end