diff options
author | Rémy Coutable <remy@rymai.me> | 2016-04-19 13:03:28 +0200 |
---|---|---|
committer | Robert Speicher <rspeicher@gmail.com> | 2016-04-20 15:43:32 -0400 |
commit | 6cdf4acd4eef55c616d633757772c6e05f6b93c5 (patch) | |
tree | 10fb609f5a9418c3f57ef3506b95f0be7c06e14c | |
parent | 5c8959bbc15ec47d8be62e8e1362e441cdd91d69 (diff) | |
download | gitlab-ce-6cdf4acd4eef55c616d633757772c6e05f6b93c5.tar.gz |
Address MR feedback
Signed-off-by: Rémy Coutable <remy@rymai.me>
-rw-r--r-- | CHANGELOG | 8 | ||||
-rw-r--r-- | app/finders/issuable_finder.rb | 11 | ||||
-rw-r--r-- | app/helpers/issues_helper.rb | 13 | ||||
-rw-r--r-- | app/models/concerns/issuable.rb | 8 | ||||
-rw-r--r-- | app/models/concerns/sortable.rb | 4 | ||||
-rw-r--r-- | app/models/issue.rb | 18 | ||||
-rw-r--r-- | app/views/shared/_sort_dropdown.html.haml | 9 | ||||
-rw-r--r-- | app/views/shared/issuable/_sidebar.html.haml | 22 | ||||
-rw-r--r-- | db/migrate/20160310124959_add_due_date_to_issues.rb | 1 | ||||
-rw-r--r-- | db/migrate/20160320123111_add_index_to_issues_due_date.rb | 5 | ||||
-rw-r--r-- | db/schema.rb | 13 | ||||
-rw-r--r-- | spec/features/issues_spec.rb | 73 |
12 files changed, 83 insertions, 102 deletions
diff --git a/CHANGELOG b/CHANGELOG index 330b94c75cf..661ca39403f 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -19,6 +19,7 @@ v 8.7.0 (unreleased) - Load award emoji images separately unless opening the full picker. Saves several hundred KBs of data for most pages. (Connor Shea) - Do not include award_emojis in issue and merge_request comment_count !3610 (Lucas Charles) - Restrict user profiles when public visibility level is restricted. + - Add ability set due date to issues, sort and filter issues by due date (Mehmet Beydogan) - All images in discussions and wikis now link to their source files !3464 (Connor Shea). - Return status code 303 after a branch DELETE operation to avoid project deletion (Stan Hu) - Add setting for customizing the list of trusted proxies !3524 @@ -188,12 +189,6 @@ v 8.6.1 - Fixes issue with assign milestone not loading milestone list. !3346 - Fix an issue causing the Dashboard/Milestones page to be blank. !3348 -v 8.6.0 - - Add ability to move issue to another project - - Prevent tokens in the import URL to be showed by the UI -v 8.7.0(unreleased) - - Add ability set due date to issues, sort and filter issues by due date(Mehmet Beydogan) - v 8.6.0 (unreleased) - Fix bug where wrong commit ID was being used in a merge request diff to show old image (Stan Hu) - Add confidential issues @@ -271,7 +266,6 @@ v 8.5.7 v 8.5.6 - Obtain a lease before querying LDAP - - Add ability set due date to issues, sort and filter issues by due date v 8.5.5 - Ensure removing a project removes associated Todo entries diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 7613283443a..e21cfbd0d47 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -122,7 +122,7 @@ class IssuableFinder end def filter_by_overdue? - due_date? && params[:due_date] == Issue::OverDue.name + due_date? && params[:due_date] == Issue::Overdue.name end def filter_by_due_this_week? @@ -305,17 +305,20 @@ class IssuableFinder end def by_due_date(items) + return items unless klass.column_names.include?('due_date') + if due_date? if filter_by_no_due_date? items = items.without_due_date elsif filter_by_overdue? - items = items.overdue + items = items.due_before(Date.today) elsif filter_by_due_this_week? - items = items.due_between(Date.today.beginning_of_week, Date.today.end_of_week + 1.day) + items = items.due_between(Date.today.beginning_of_week, Date.today.end_of_week) elsif filter_by_due_this_month? - items = items.due_between(Date.today.beginning_of_month, Date.today.end_of_month + 1.day) + items = items.due_between(Date.today.beginning_of_month, Date.today.end_of_month) end end + items end diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb index d3779eda91f..afe1e11a0da 100644 --- a/app/helpers/issues_helper.rb +++ b/app/helpers/issues_helper.rb @@ -131,7 +131,7 @@ module IssuesHelper class: "icon emoji-icon emoji-#{unicode}", title: name, data: data - else + else # Emoji icons displayed separately, used for the awards already given # to an issue or merge request. content_tag :img, "", @@ -174,17 +174,16 @@ module IssuesHelper def due_date_options options = [ - Issue::AnyDueDate, - Issue::NoDueDate, - Issue::DueThisWeek, - Issue::DueThisMonth, - Issue::OverDue + Issue::AnyDueDate, + Issue::NoDueDate, + Issue::DueThisWeek, + Issue::DueThisMonth, + Issue::Overdue ] options_from_collection_for_select(options, 'name', 'title', params[:due_date]) end - # Required for Banzai::Filter::IssueReferenceFilter module_function :url_for_issue end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 03d8a573a4d..7c2a92fa86a 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -39,10 +39,10 @@ module Issuable scope :order_milestone_due_asc, -> { joins(:milestone).reorder('milestones.due_date ASC, milestones.id ASC') } scope :with_label, ->(title) { joins(:labels).where(labels: { title: title }) } scope :without_label, -> { joins("LEFT OUTER JOIN label_links ON label_links.target_type = '#{name}' AND label_links.target_id = #{table_name}.id").where(label_links: { id: nil }) } - scope :without_due_date, ->{ where("issues.due_date IS NULL")} - scope :due_before, ->(date){ where("issues.due_date IS NOT NULL AND issues.due_date < ?", date)} - scope :due_between, ->(from_date, to_date){ where("issues.due_date >= ?", from_date).due_before(to_date) } - scope :overdue, ->{ where("issues.due_date < ?", Date.today)} + + scope :without_due_date, -> { where('issues.due_date IS NULL') } + scope :due_before, ->(date) { where('issues.due_date < ?', date) } + scope :due_between, ->(from_date, to_date) { where('issues.due_date >= ?', from_date).where('issues.due_date <= ?', to_date) } scope :join_project, -> { joins(:project) } scope :references_project, -> { references(:project) } diff --git a/app/models/concerns/sortable.rb b/app/models/concerns/sortable.rb index f1f09011c1b..42617ecbb96 100644 --- a/app/models/concerns/sortable.rb +++ b/app/models/concerns/sortable.rb @@ -33,8 +33,8 @@ module Sortable when 'created_desc' then order_created_desc when 'id_desc' then order_id_desc when 'id_asc' then order_id_asc - when 'due_date_asc' then order_due_date_asc - when 'due_date_desc' then order_due_date_desc + when 'due_date_asc', 'due_date_desc' + column_names.include?('due_date') ? send("order_#{method}") : all else all end diff --git a/app/models/issue.rb b/app/models/issue.rb index db0208ae3ad..d72233006ee 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -28,12 +28,12 @@ class Issue < ActiveRecord::Base include Sortable include Taskable - DueDateStruct = Struct.new(:title, :name) - NoDueDate = DueDateStruct.new('No Due Date', '0') - AnyDueDate = DueDateStruct.new('Any Due Date', '') - OverDue = DueDateStruct.new('Overdue', 'overdue') - DueThisWeek = DueDateStruct.new('Due This Week', 'week') - DueThisMonth = DueDateStruct.new('Due This Month', 'month') + DueDateStruct = Struct.new(:title, :name).freeze + NoDueDate = DueDateStruct.new('No Due Date', '0').freeze + AnyDueDate = DueDateStruct.new('Any Due Date', '').freeze + Overdue = DueDateStruct.new('Overdue', 'overdue').freeze + DueThisWeek = DueDateStruct.new('Due This Week', 'week').freeze + DueThisMonth = DueDateStruct.new('Due This Month', 'month').freeze ActsAsTaggableOn.strict_case_match = true @@ -178,10 +178,6 @@ class Issue < ActiveRecord::Base end def overdue? - if due_date - due_date.past? - else - false - end + due_date.try(:past?) || false end end diff --git a/app/views/shared/_sort_dropdown.html.haml b/app/views/shared/_sort_dropdown.html.haml index 922b02f9f3e..d327bd0a96f 100644 --- a/app/views/shared/_sort_dropdown.html.haml +++ b/app/views/shared/_sort_dropdown.html.haml @@ -20,10 +20,11 @@ = sort_title_milestone_soon = link_to page_filter_path(sort: sort_value_milestone_later) do = sort_title_milestone_later - = link_to page_filter_path(sort: sort_value_due_date_soon) do - = sort_title_due_date_soon if @issues - = link_to page_filter_path(sort: sort_value_due_date_later) do - = sort_title_due_date_later if @issues + - if controller.controller_name == 'issues' || controller.action_name == 'issues' + = link_to page_filter_path(sort: sort_value_due_date_soon) do + = sort_title_due_date_soon + = link_to page_filter_path(sort: sort_value_due_date_later) do + = sort_title_due_date_later = link_to page_filter_path(sort: sort_value_upvotes) do = sort_title_upvotes = link_to page_filter_path(sort: sort_value_downvotes) do diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml index 61a655a40df..a4dbb5d23fc 100644 --- a/app/views/shared/issuable/_sidebar.html.haml +++ b/app/views/shared/issuable/_sidebar.html.haml @@ -58,7 +58,7 @@ - if issuable.milestone = issuable.milestone.title - else - No + None .title.hide-collapsed Milestone = icon('spinner spin', class: 'block-loading') @@ -74,17 +74,15 @@ .selectbox.hide-collapsed = f.hidden_field 'milestone_id', value: issuable.milestone_id, id: nil = dropdown_tag('Milestone', options: { title: 'Assign milestone', toggle_class: 'js-milestone-select js-extra-options', filter: true, dropdown_class: 'dropdown-menu-selectable', placeholder: 'Search milestones', data: { show_no: true, field_name: "#{issuable.to_ability_name}[milestone_id]", project_id: @project.id, issuable_id: issuable.id, milestones: namespace_project_milestones_path(@project.namespace, @project, :json), ability_name: issuable.to_ability_name, issue_update: issuable_json_path(issuable), use_id: true }}) - - if issuable.has_attribute? :due_date + + - if issuable.has_attribute?(:due_date) .block.due_date .sidebar-collapsed-icon = icon('calendar') %span.js-due-date-sidebar-value - - if issuable.due_date - = issuable.due_date.to_s(:medium) - - else - None + = issuable.due_date.try(:to_s, :medium) || 'None' .title.hide-collapsed - Due Date + Due date = icon('spinner spin', class: 'block-loading') - if can?(current_user, :"admin_#{issuable.to_ability_name}", @project) = link_to 'Edit', '#', class: 'edit-link pull-right' @@ -96,13 +94,13 @@ - if can?(current_user, :"admin_#{issuable.to_ability_name}", @project) .selectbox.hide-collapsed = hidden_field_tag "#{issuable.to_ability_name}[due_date]", issuable.due_date + = f.hidden_field :due_date, value: issuable.due_date .dropdown - %button.dropdown-menu-toggle.js-due-date-select{ type: "button", data: { toggle: "dropdown", field_name: "#{issuable.to_ability_name}[due_date]", ability_name: issuable.to_ability_name, issue_update: issuable_json_path(issuable) } } - %span.dropdown-toggle-text - Due date - = icon("chevron-down") + %button.dropdown-menu-toggle.js-due-date-select{ type: 'button', data: { toggle: 'dropdown', field_name: "#{issuable.to_ability_name}[due_date]", ability_name: issuable.to_ability_name, issue_update: issuable_json_path(issuable) } } + %span.dropdown-toggle-text Due date + = icon('chevron-down') .dropdown-menu.dropdown-menu-due-date - = dropdown_title("Due date") + = dropdown_title('Due date') = dropdown_content do .js-due-date-calendar diff --git a/db/migrate/20160310124959_add_due_date_to_issues.rb b/db/migrate/20160310124959_add_due_date_to_issues.rb index c232387a6f3..ec08bd9fdfa 100644 --- a/db/migrate/20160310124959_add_due_date_to_issues.rb +++ b/db/migrate/20160310124959_add_due_date_to_issues.rb @@ -1,5 +1,6 @@ class AddDueDateToIssues < ActiveRecord::Migration def change add_column :issues, :due_date, :date + add_index :issues, :due_date end end diff --git a/db/migrate/20160320123111_add_index_to_issues_due_date.rb b/db/migrate/20160320123111_add_index_to_issues_due_date.rb deleted file mode 100644 index 03a33c679f4..00000000000 --- a/db/migrate/20160320123111_add_index_to_issues_due_date.rb +++ /dev/null @@ -1,5 +0,0 @@ -class AddIndexToIssuesDueDate < ActiveRecord::Migration - def change - add_index :issues, :due_date - end -end diff --git a/db/schema.rb b/db/schema.rb index 699a99c0743..a93ba690730 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -366,19 +366,6 @@ ActiveRecord::Schema.define(version: 20160419120017) do add_index "emails", ["email"], name: "index_emails_on_email", unique: true, using: :btree add_index "emails", ["user_id"], name: "index_emails_on_user_id", using: :btree - create_table "emoji_awards", force: :cascade do |t| - t.string "name" - t.integer "user_id" - t.integer "awardable_id" - t.string "awardable_type" - t.datetime "created_at" - t.datetime "updated_at" - end - - add_index "emoji_awards", ["awardable_id"], name: "index_emoji_awards_on_awardable_id", using: :btree - add_index "emoji_awards", ["awardable_type"], name: "index_emoji_awards_on_awardable_type", using: :btree - add_index "emoji_awards", ["user_id"], name: "index_emoji_awards_on_user_id", using: :btree - create_table "events", force: :cascade do |t| t.string "target_type" t.integer "target_id" diff --git a/spec/features/issues_spec.rb b/spec/features/issues_spec.rb index 32c27d6e97e..5d38bdeba33 100644 --- a/spec/features/issues_spec.rb +++ b/spec/features/issues_spec.rb @@ -112,7 +112,7 @@ describe 'Issues', feature: true do end describe 'filter issue' do - titles = ['foo','bar','baz'] + titles = %w[foo bar baz] titles.each_with_index do |title, index| let!(title.to_sym) do create(:issue, title: title, @@ -154,86 +154,93 @@ describe 'Issues', feature: true do end describe 'sorting by due date' do - before :each do - foo.due_date = 1.day.from_now - foo.save - bar.due_date = 6.days.from_now - bar.save + before do + foo.update(due_date: 1.day.from_now) + bar.update(due_date: 6.days.from_now) end it 'sorts by recently due date' do visit namespace_project_issues_path(project.namespace, project, sort: sort_value_due_date_soon) + expect(first_issue).to include('foo') end it 'sorts by least recently due date' do visit namespace_project_issues_path(project.namespace, project, sort: sort_value_due_date_later) + expect(first_issue).to include('bar') end it 'sorts by least recently due date by excluding nil due dates' do bar.update(due_date: nil) + visit namespace_project_issues_path(project.namespace, project, sort: sort_value_due_date_later) + expect(first_issue).to include('foo') end end describe 'filtering by due date' do - before :each do - foo.due_date = 1.day.from_now - foo.save - bar.due_date = 6.days.from_now - bar.save + before do + foo.update(due_date: 1.day.from_now) + bar.update(due_date: 6.days.from_now) end it 'filters by none' do visit namespace_project_issues_path(project.namespace, project, due_date: Issue::NoDueDate.name) - expect(page).not_to have_content("foo") - expect(page).not_to have_content("bar") - expect(page).to have_content("baz") + + expect(page).not_to have_content('foo') + expect(page).not_to have_content('bar') + expect(page).to have_content('baz') end it 'filters by any' do visit namespace_project_issues_path(project.namespace, project, due_date: Issue::AnyDueDate.name) - expect(page).to have_content("foo") - expect(page).to have_content("bar") - expect(page).to have_content("baz") + + expect(page).to have_content('foo') + expect(page).to have_content('bar') + expect(page).to have_content('baz') end it 'filters by due this week' do foo.update(due_date: Date.today.beginning_of_week + 2.days) bar.update(due_date: Date.today.end_of_week) baz.update(due_date: Date.today - 8.days) + visit namespace_project_issues_path(project.namespace, project, due_date: Issue::DueThisWeek.name) - expect(page).to have_content("foo") - expect(page).to have_content("bar") - expect(page).not_to have_content("baz") + + expect(page).to have_content('foo') + expect(page).to have_content('bar') + expect(page).not_to have_content('baz') end it 'filters by due this month' do foo.update(due_date: Date.today.beginning_of_month + 2.days) bar.update(due_date: Date.today.end_of_month) baz.update(due_date: Date.today - 50.days) + visit namespace_project_issues_path(project.namespace, project, due_date: Issue::DueThisMonth.name) - expect(page).to have_content("foo") - expect(page).to have_content("bar") - expect(page).not_to have_content("baz") + + expect(page).to have_content('foo') + expect(page).to have_content('bar') + expect(page).not_to have_content('baz') end it 'filters by overdue' do foo.update(due_date: Date.today + 2.days) bar.update(due_date: Date.today + 20.days) baz.update(due_date: Date.yesterday) - visit namespace_project_issues_path(project.namespace, project, due_date: Issue::OverDue.name) - expect(page).not_to have_content("foo") - expect(page).not_to have_content("bar") - expect(page).to have_content("baz") - end + visit namespace_project_issues_path(project.namespace, project, due_date: Issue::Overdue.name) + + expect(page).not_to have_content('foo') + expect(page).not_to have_content('bar') + expect(page).to have_content('baz') + end end describe 'sorting by milestone' do - before :each do + before do foo.milestone = newer_due_milestone foo.save bar.milestone = later_due_milestone @@ -256,7 +263,7 @@ describe 'Issues', feature: true do describe 'combine filter and sort' do let(:user2) { create(:user) } - before :each do + before do foo.assignee = user2 foo.save bar.assignee = user2 @@ -303,7 +310,7 @@ describe 'Issues', feature: true do let(:guest) { create(:user) } - before :each do + before do project.team << [[guest], :guest] end @@ -346,7 +353,7 @@ describe 'Issues', feature: true do context 'by unauthorized user' do let(:guest) { create(:user) } - before :each do + before do project.team << [guest, :guest] issue.milestone = milestone issue.save @@ -364,7 +371,7 @@ describe 'Issues', feature: true do describe 'removing assignee' do let(:user2) { create(:user) } - before :each do + before do issue.assignee = user2 issue.save end |