From 41d8f5649e3c8a1e37be4608fd03153005c3fa58 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Fri, 12 Feb 2016 11:17:42 -0200 Subject: [PATCH 01/50] Add task model --- app/models/task.rb | 29 ++++++++++++++++++++ app/models/user.rb | 2 +- db/migrate/20160212123307_create_tasks.rb | 14 ++++++++++ db/schema.rb | 18 +++++++++++++ spec/models/task_spec.rb | 32 +++++++++++++++++++++++ spec/models/user_spec.rb | 1 + 6 files changed, 95 insertions(+), 1 deletion(-) create mode 100644 app/models/task.rb create mode 100644 db/migrate/20160212123307_create_tasks.rb create mode 100644 spec/models/task_spec.rb diff --git a/app/models/task.rb b/app/models/task.rb new file mode 100644 index 00000000000..5f102dd66b8 --- /dev/null +++ b/app/models/task.rb @@ -0,0 +1,29 @@ +# == Schema Information +# +# Table name: tasks +# +# id :integer not null, primary key +# user_id :integer not null +# project_id :integer not null +# target_id :integer not null +# target_type :string not null +# author_id :integer +# action :integer +# state :string not null +# created_at :datetime +# updated_at :datetime +# + +class Task < ActiveRecord::Base + belongs_to :author, class_name: "User" + belongs_to :project + belongs_to :target, polymorphic: true, touch: true + belongs_to :user + + validates :action, :project, :target, :user, presence: true + + state_machine :state, initial: :pending do + state :pending + state :done + end +end diff --git a/app/models/user.rb b/app/models/user.rb index 9fe94b13e52..d108ba78e4b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -140,7 +140,7 @@ class User < ActiveRecord::Base has_one :abuse_report, dependent: :destroy has_many :spam_logs, dependent: :destroy has_many :builds, dependent: :nullify, class_name: 'Ci::Build' - + has_many :tasks, dependent: :destroy # # Validations diff --git a/db/migrate/20160212123307_create_tasks.rb b/db/migrate/20160212123307_create_tasks.rb new file mode 100644 index 00000000000..c3f6f3abc26 --- /dev/null +++ b/db/migrate/20160212123307_create_tasks.rb @@ -0,0 +1,14 @@ +class CreateTasks < ActiveRecord::Migration + def change + create_table :tasks do |t| + t.references :user, null: false, index: true + t.references :project, null: false, index: true + t.references :target, polymorphic: true, null: false, index: true + t.integer :author_id, index: true + t.integer :action, null: false + t.string :state, null: false, index: true + + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index af5bac63b42..183227a91ca 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -824,6 +824,24 @@ ActiveRecord::Schema.define(version: 20160217100506) do add_index "tags", ["name"], name: "index_tags_on_name", unique: true, using: :btree + create_table "tasks", force: :cascade do |t| + t.integer "user_id", null: false + t.integer "project_id", null: false + t.integer "target_id", null: false + t.string "target_type", null: false + t.integer "author_id" + t.integer "action", null: false + t.string "state", null: false + t.datetime "created_at" + t.datetime "updated_at" + end + + add_index "tasks", ["author_id"], name: "index_tasks_on_author_id", using: :btree + add_index "tasks", ["project_id"], name: "index_tasks_on_project_id", using: :btree + add_index "tasks", ["state"], name: "index_tasks_on_state", using: :btree + add_index "tasks", ["target_type", "target_id"], name: "index_tasks_on_target_type_and_target_id", using: :btree + add_index "tasks", ["user_id"], name: "index_tasks_on_user_id", using: :btree + create_table "users", force: :cascade do |t| t.string "email", default: "", null: false t.string "encrypted_password", default: "", null: false diff --git a/spec/models/task_spec.rb b/spec/models/task_spec.rb new file mode 100644 index 00000000000..950d9b74196 --- /dev/null +++ b/spec/models/task_spec.rb @@ -0,0 +1,32 @@ +# == Schema Information +# +# Table name: tasks +# +# id :integer not null, primary key +# user_id :integer not null +# project_id :integer not null +# target_id :integer not null +# target_type :string not null +# author_id :integer +# action :integer +# state :string not null +# created_at :datetime +# updated_at :datetime +# + +require 'spec_helper' + +describe Task, models: true do + describe 'relationships' do + it { is_expected.to belong_to(:author).class_name("User") } + it { is_expected.to belong_to(:project) } + it { is_expected.to belong_to(:target).touch(true) } + it { is_expected.to belong_to(:user) } + end + + describe 'validations' do + it { is_expected.to validate_presence_of(:action) } + it { is_expected.to validate_presence_of(:target) } + it { is_expected.to validate_presence_of(:user) } + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 47ce409fe4b..d2769836526 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -92,6 +92,7 @@ describe User, models: true do it { is_expected.to have_many(:identities).dependent(:destroy) } it { is_expected.to have_one(:abuse_report) } it { is_expected.to have_many(:spam_logs).dependent(:destroy) } + it { is_expected.to have_many(:tasks).dependent(:destroy) } end describe 'validations' do -- 2.18.1 From 7cafa2ce923df9c034ba0b64907b4b6eb6b3c1e2 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Fri, 12 Feb 2016 16:45:44 -0200 Subject: [PATCH 02/50] Add tasks queue list page --- app/assets/stylesheets/pages/tasks.scss | 90 +++++++++++++++++++ app/controllers/dashboard/tasks_controller.rb | 15 ++++ app/helpers/tasks_helper.rb | 17 ++++ app/models/task.rb | 23 +++++ app/views/dashboard/tasks/_task.html.haml | 17 ++++ app/views/dashboard/tasks/index.html.haml | 25 ++++++ config/routes.rb | 1 + spec/models/task_spec.rb | 23 +++++ 8 files changed, 211 insertions(+) create mode 100644 app/assets/stylesheets/pages/tasks.scss create mode 100644 app/controllers/dashboard/tasks_controller.rb create mode 100644 app/helpers/tasks_helper.rb create mode 100644 app/views/dashboard/tasks/_task.html.haml create mode 100644 app/views/dashboard/tasks/index.html.haml diff --git a/app/assets/stylesheets/pages/tasks.scss b/app/assets/stylesheets/pages/tasks.scss new file mode 100644 index 00000000000..702d7c55e59 --- /dev/null +++ b/app/assets/stylesheets/pages/tasks.scss @@ -0,0 +1,90 @@ +/** + * Dashboard tasks queue + * + */ +.tasks { + .panel { + border-top: none; + margin-bottom: 0; + } +} + +.task-item { + font-size: $gl-font-size; + padding: $gl-padding-top 0 $gl-padding-top ($gl-avatar-size + $gl-padding-top); + border-bottom: 1px solid $table-border-color; + color: #7f8fa4; + + &.task-inline { + .avatar { + position: relative; + top: -2px; + } + + .task-title { + line-height: 40px; + } + } + + a { + color: #4c4e54; + } + + .avatar { + margin-left: -($gl-avatar-size + $gl-padding-top); + } + + .task-title { + @include str-truncated(calc(100% - 174px)); + font-weight: 600; + + .author_name { + color: #333; + } + } + + .task-body { + margin-right: 174px; + + .task-note { + word-wrap: break-word; + + pre { + border: none; + background: #f9f9f9; + border-radius: 0; + color: #777; + margin: 0 20px; + overflow: hidden; + } + + p:last-child { + margin-bottom: 0; + } + } + } + + &:last-child { border:none } +} + +@media (max-width: $screen-xs-max) { + .task-item { + padding-left: $gl-padding; + + .task-title { + white-space: normal; + overflow: visible; + max-width: 100%; + } + + .avatar { + display: none; + } + + .task-body { + margin: 0; + border-left: 2px solid #DDD; + padding-left: 10px; + } + } +} diff --git a/app/controllers/dashboard/tasks_controller.rb b/app/controllers/dashboard/tasks_controller.rb new file mode 100644 index 00000000000..66d891e3dfa --- /dev/null +++ b/app/controllers/dashboard/tasks_controller.rb @@ -0,0 +1,15 @@ +class Dashboard::TasksController < Dashboard::ApplicationController + def index + @tasks = case params[:state] + when 'done' + current_user.tasks.done + else + current_user.tasks.pending + end + + @tasks = @tasks.page(params[:page]).per(PER_PAGE) + + @pending_count = current_user.tasks.pending.count + @done_count = current_user.tasks.done.count + end +end diff --git a/app/helpers/tasks_helper.rb b/app/helpers/tasks_helper.rb new file mode 100644 index 00000000000..4a87f8d4ca4 --- /dev/null +++ b/app/helpers/tasks_helper.rb @@ -0,0 +1,17 @@ +module TasksHelper + def link_to_author(task) + author = task.author + + if author + link_to author.name, user_path(author.username) + else + task.author_name + end + end + + def task_action_name(task) + target = task.target_type.titleize.downcase + + [task.action_name, target].join(" ") + end +end diff --git a/app/models/task.rb b/app/models/task.rb index 5f102dd66b8..37e752bd350 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -15,15 +15,38 @@ # class Task < ActiveRecord::Base + ASSIGNED = 1 + belongs_to :author, class_name: "User" belongs_to :project belongs_to :target, polymorphic: true, touch: true belongs_to :user + delegate :name, :email, to: :author, prefix: true, allow_nil: true + validates :action, :project, :target, :user, presence: true + default_scope { reorder(id: :desc) } + + scope :pending, -> { with_state(:pending) } + scope :done, -> { with_state(:done) } + state_machine :state, initial: :pending do state :pending state :done end + + def action_name + case action + when ASSIGNED then 'assigned' + end + end + + def body? + target.respond_to? :title + end + + def target_iid + target.respond_to?(:iid) ? target.iid : target_id + end end diff --git a/app/views/dashboard/tasks/_task.html.haml b/app/views/dashboard/tasks/_task.html.haml new file mode 100644 index 00000000000..c7b952f0cb5 --- /dev/null +++ b/app/views/dashboard/tasks/_task.html.haml @@ -0,0 +1,17 @@ +%li{class: "task task-#{task.done? ? 'done' : 'pending'}", id: dom_id(task) } + .task-item{class: "#{task.body? ? 'task-block' : 'task-inline' }"} + = image_tag avatar_icon(task.author_email, 40), class: "avatar s40", alt:'' + + .task-title + %span.author_name= link_to_author task + %span.task_label{class: task.action_name} + = task_action_name(task) + + %strong= link_to "##{task.target_iid}", [task.project.namespace.becomes(Namespace), task.project, task.target] + + · #{time_ago_with_tooltip(task.created_at)} + + - if task.body? + .task-body + .task-note + = task.target.title diff --git a/app/views/dashboard/tasks/index.html.haml b/app/views/dashboard/tasks/index.html.haml new file mode 100644 index 00000000000..877c69185c4 --- /dev/null +++ b/app/views/dashboard/tasks/index.html.haml @@ -0,0 +1,25 @@ +- page_title "Tasks Queue" +- header_title "Tasks Queue", dashboard_tasks_path + +.top-area + %ul.nav-links + %li{class: ("active" if params[:state].blank? || params[:state] == 'pending')} + = link_to dashboard_tasks_path(state: 'pending') do + Tasks (#{@pending_count}) + %li{class: ("active" if params[:state] == 'done')} + = link_to dashboard_tasks_path(state: 'done') do + Done (#{@done_count}) + +.tasks + - if @tasks.any? + - @tasks.group_by(&:project).each do |group| + .panel.panel-default + - project = group[0] + .panel-heading + = link_to project.name_with_namespace, namespace_project_path(project.namespace, project) + + %ul.well-list.tasks-list + = render group[1] + = paginate @tasks, theme: "gitlab" + - else + .nothing-here-block No tasks to show diff --git a/config/routes.rb b/config/routes.rb index 0ed3f1731f8..17c0bb22a8c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -333,6 +333,7 @@ Rails.application.routes.draw do resources :groups, only: [:index] resources :snippets, only: [:index] + resources :tasks, only: [:index] resources :projects, only: [:index] do collection do diff --git a/spec/models/task_spec.rb b/spec/models/task_spec.rb index 950d9b74196..86317626cc3 100644 --- a/spec/models/task_spec.rb +++ b/spec/models/task_spec.rb @@ -24,9 +24,32 @@ describe Task, models: true do it { is_expected.to belong_to(:user) } end + describe 'respond to' do + it { is_expected.to respond_to(:author_name) } + it { is_expected.to respond_to(:author_email) } + end + describe 'validations' do it { is_expected.to validate_presence_of(:action) } it { is_expected.to validate_presence_of(:target) } it { is_expected.to validate_presence_of(:user) } end + + describe '#action_name' do + it 'returns assigned when action is assigned' do + subject.action = Task::ASSIGNED + + expect(subject.action_name).to eq 'assigned' + end + end + + describe '#body?' do + it 'returns true when target respond to title' + it 'returns false when target does not respond to title' + end + + describe '#target_iid' do + it 'returns target.iid when target respond to iid' + it 'returns target_id when target does not respond to iid' + end end -- 2.18.1 From 1e0053f2dca63fdba18811a2194a36f828d45486 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 15 Feb 2016 13:13:52 -0200 Subject: [PATCH 03/50] Fix task queue list page title --- app/views/dashboard/tasks/index.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/dashboard/tasks/index.html.haml b/app/views/dashboard/tasks/index.html.haml index 877c69185c4..8147d017c3f 100644 --- a/app/views/dashboard/tasks/index.html.haml +++ b/app/views/dashboard/tasks/index.html.haml @@ -1,5 +1,5 @@ -- page_title "Tasks Queue" -- header_title "Tasks Queue", dashboard_tasks_path +- page_title "Task Queue" +- header_title "Task Queue", dashboard_tasks_path .top-area %ul.nav-links -- 2.18.1 From 422a01fc853a93e8946c69dc2ad0ad1dea261653 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 15 Feb 2016 16:13:52 -0200 Subject: [PATCH 04/50] Create a pending task when an issue is assigned to someone --- app/services/base_service.rb | 4 ++ app/services/issues/create_service.rb | 1 + app/services/issues/update_service.rb | 1 + app/services/task_service.rb | 43 +++++++++++++++ spec/services/issues/create_service_spec.rb | 22 +++++++- spec/services/issues/update_service_spec.rb | 13 +++++ spec/services/task_service_spec.rb | 59 +++++++++++++++++++++ 7 files changed, 141 insertions(+), 2 deletions(-) create mode 100644 app/services/task_service.rb create mode 100644 spec/services/task_service_spec.rb diff --git a/app/services/base_service.rb b/app/services/base_service.rb index b48ca67d4d2..c349997b9e4 100644 --- a/app/services/base_service.rb +++ b/app/services/base_service.rb @@ -23,6 +23,10 @@ class BaseService EventCreateService.new end + def task_service + TaskService.new + end + def log_info(message) Gitlab::AppLogger.info message end diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index bcb380d3215..2a6c84c3ce5 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -9,6 +9,7 @@ module Issues if issue.save issue.update_attributes(label_ids: label_params) notification_service.new_issue(issue, current_user) + task_service.new_issue(issue, current_user) event_service.open_issue(issue, current_user) issue.create_cross_references!(current_user) execute_hooks(issue, 'open') diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index a55a04dd5e0..e6afcb91652 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -12,6 +12,7 @@ module Issues if issue.previous_changes.include?('assignee_id') create_assignee_note(issue) notification_service.reassigned_issue(issue, current_user) + task_service.reassigned_issue(issue, current_user) end end diff --git a/app/services/task_service.rb b/app/services/task_service.rb new file mode 100644 index 00000000000..d82bce10eda --- /dev/null +++ b/app/services/task_service.rb @@ -0,0 +1,43 @@ +# TaskService class +# +# Used for creating tasks on task queue after certain user action +# +# Ex. +# TaskService.new.new_issue(issue, current_user) +# +class TaskService + # When create an issue we should: + # + # * creates a pending task for assignee if issue is assigned + # + def new_issue(issue, current_user) + if issue.is_assigned? + create_task(issue.project, issue, current_user, issue.assignee, Task::ASSIGNED) + end + end + + # When we reassign an issue we should: + # + # * creates a pending task for new assignee if issue is assigned + # + def reassigned_issue(issue, current_user) + if issue.is_assigned? + create_task(issue.project, issue, current_user, issue.assignee, Task::ASSIGNED) + end + end + + private + + def create_task(project, target, author, user, action) + attributes = { + project: project, + user_id: user.id, + author_id: author.id, + target_id: target.id, + target_type: target.class.name, + action: action + } + + Task.create(attributes) + end +end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 2148d091a57..f3b66779987 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -3,14 +3,18 @@ require 'spec_helper' describe Issues::CreateService, services: true do let(:project) { create(:empty_project) } let(:user) { create(:user) } + let(:assignee) { create(:user) } describe :execute do - context "valid params" do + context 'valid params' do before do project.team << [user, :master] + project.team << [assignee, :master] + opts = { title: 'Awesome issue', - description: 'please fix' + description: 'please fix', + assignee: assignee } @issue = Issues::CreateService.new(project, user, opts).execute @@ -18,6 +22,20 @@ describe Issues::CreateService, services: true do it { expect(@issue).to be_valid } it { expect(@issue.title).to eq('Awesome issue') } + it { expect(@issue.assignee).to eq assignee } + + it 'creates a pending task for new assignee' do + attributes = { + project: project, + author: user, + user: assignee, + target: @issue, + action: Task::ASSIGNED, + state: :pending + } + + expect(Task.where(attributes).count).to eq 1 + end end end end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 87da0e9618b..8f654517bce 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -78,6 +78,19 @@ describe Issues::UpdateService, services: true do expect(note).not_to be_nil expect(note.note).to eq 'Title changed from **Old title** to **New title**' end + + it 'creates a pending task if being reassigned' do + attributes = { + project: project, + author: user, + user: user2, + target: issue, + action: Task::ASSIGNED, + state: :pending + } + + expect(Task.where(attributes).count).to eq 1 + end end context 'when Issue has tasks' do diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb new file mode 100644 index 00000000000..ee3c4f8f95d --- /dev/null +++ b/spec/services/task_service_spec.rb @@ -0,0 +1,59 @@ +require 'spec_helper' + +describe TaskService, services: true do + let(:service) { described_class.new } + + describe 'Issues' do + let(:author) { create(:user) } + let(:john_doe) { create(:user) } + let(:project) { create(:empty_project, :public) } + let(:assigned_issue) { create(:issue, project: project, assignee: john_doe) } + let(:unassigned_issue) { create(:issue, project: project, assignee: nil) } + + before do + project.team << [author, :developer] + project.team << [john_doe, :developer] + end + + describe '#new_issue' do + it 'creates a pending task if assigned' do + service.new_issue(assigned_issue, author) + + is_expected_to_create_pending_task(user: john_doe, target: assigned_issue, action: Task::ASSIGNED) + end + + it 'does not create a task if unassigned' do + is_expected_to_not_create_task { service.new_issue(unassigned_issue, author) } + end + end + + describe '#reassigned_issue' do + it 'creates a pending task for new assignee' do + unassigned_issue.update_attribute(:assignee, john_doe) + service.reassigned_issue(unassigned_issue, author) + + is_expected_to_create_pending_task(user: john_doe, target: unassigned_issue, action: Task::ASSIGNED) + end + + it 'does not create a task if unassigned' do + assigned_issue.update_attribute(:assignee, nil) + + is_expected_to_not_create_task { service.reassigned_issue(assigned_issue, author) } + end + end + end + + def is_expected_to_create_pending_task(attributes = {}) + attributes.reverse_merge!( + project: project, + author: author, + state: :pending + ) + + expect(Task.where(attributes).count).to eq 1 + end + + def is_expected_to_not_create_task + expect { yield }.not_to change(Task, :count) + end +end -- 2.18.1 From c8f2d18abde050c3b6d15ee32c99495c77b2a222 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 15 Feb 2016 16:55:03 -0200 Subject: [PATCH 05/50] Does not create a task for new issue when assignee is the current user --- app/services/task_service.rb | 2 +- spec/services/task_service_spec.rb | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/app/services/task_service.rb b/app/services/task_service.rb index d82bce10eda..35d258a70c2 100644 --- a/app/services/task_service.rb +++ b/app/services/task_service.rb @@ -11,7 +11,7 @@ class TaskService # * creates a pending task for assignee if issue is assigned # def new_issue(issue, current_user) - if issue.is_assigned? + if issue.is_assigned? && issue.assignee != current_user create_task(issue.project, issue, current_user, issue.assignee, Task::ASSIGNED) end end diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb index ee3c4f8f95d..db9f77fd12d 100644 --- a/spec/services/task_service_spec.rb +++ b/spec/services/task_service_spec.rb @@ -25,6 +25,10 @@ describe TaskService, services: true do it 'does not create a task if unassigned' do is_expected_to_not_create_task { service.new_issue(unassigned_issue, author) } end + + it 'does not create a task if assignee is the current user' do + is_expected_to_not_create_task { service.new_issue(unassigned_issue, john_doe) } + end end describe '#reassigned_issue' do -- 2.18.1 From 77d7910b8b4497dc52a80f8a35b765c978d547a4 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 15 Feb 2016 17:49:28 -0200 Subject: [PATCH 06/50] Allow user to mark each task as done manually --- app/controllers/dashboard/tasks_controller.rb | 23 +++++++++++++++++++ app/models/ability.rb | 11 +++++++++ app/models/task.rb | 4 ++++ app/views/dashboard/tasks/_task.html.haml | 4 ++++ config/routes.rb | 2 +- 5 files changed, 43 insertions(+), 1 deletion(-) diff --git a/app/controllers/dashboard/tasks_controller.rb b/app/controllers/dashboard/tasks_controller.rb index 66d891e3dfa..1102745067f 100644 --- a/app/controllers/dashboard/tasks_controller.rb +++ b/app/controllers/dashboard/tasks_controller.rb @@ -1,4 +1,6 @@ class Dashboard::TasksController < Dashboard::ApplicationController + before_action :authorize_destroy_task!, only: [:destroy] + def index @tasks = case params[:state] when 'done' @@ -12,4 +14,25 @@ class Dashboard::TasksController < Dashboard::ApplicationController @pending_count = current_user.tasks.pending.count @done_count = current_user.tasks.done.count end + + def destroy + task.done! + + respond_to do |format| + format.html { redirect_to dashboard_tasks_path, notice: 'Task was successfully marked as done.' } + format.js { render nothing: true } + end + end + + private + + def authorize_destroy_task! + unless can?(current_user, :destroy_task, task) + return render_404 + end + end + + def task + @task ||= current_user.tasks.find(params[:id]) + end end diff --git a/app/models/ability.rb b/app/models/ability.rb index a866eadeebb..5e0c76004d2 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -17,6 +17,7 @@ class Ability when Namespace then namespace_abilities(user, subject) when GroupMember then group_member_abilities(user, subject) when ProjectMember then project_member_abilities(user, subject) + when Task then task_abilities(user, subject) else [] end.concat(global_abilities(user)) end @@ -416,6 +417,16 @@ class Ability rules end + def task_abilities(user, task) + rules = [] + + if task && task.user == user + rules << :destroy_task + end + + rules + end + def abilities @abilities ||= begin abilities = Six.new diff --git a/app/models/task.rb b/app/models/task.rb index 37e752bd350..b9e5152b819 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -32,6 +32,10 @@ class Task < ActiveRecord::Base scope :done, -> { with_state(:done) } state_machine :state, initial: :pending do + event :done do + transition pending: :done + end + state :pending state :done end diff --git a/app/views/dashboard/tasks/_task.html.haml b/app/views/dashboard/tasks/_task.html.haml index c7b952f0cb5..b33f3894fd3 100644 --- a/app/views/dashboard/tasks/_task.html.haml +++ b/app/views/dashboard/tasks/_task.html.haml @@ -11,6 +11,10 @@ · #{time_ago_with_tooltip(task.created_at)} + - if task.pending? + .task-actions.pull-right + = link_to 'Done', [:dashboard, task], method: :delete, class: 'btn' + - if task.body? .task-body .task-note diff --git a/config/routes.rb b/config/routes.rb index 17c0bb22a8c..0b263933fba 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -333,7 +333,7 @@ Rails.application.routes.draw do resources :groups, only: [:index] resources :snippets, only: [:index] - resources :tasks, only: [:index] + resources :tasks, only: [:index, :destroy] resources :projects, only: [:index] do collection do -- 2.18.1 From 38026e5f7708473d2ae7a284a174fea7dac1e5db Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 15 Feb 2016 18:38:38 -0200 Subject: [PATCH 07/50] Add pending tasks badge on top right next to the new and logout button --- app/assets/stylesheets/pages/tasks.scss | 10 ++++++++++ app/controllers/dashboard/tasks_controller.rb | 3 --- app/helpers/tasks_helper.rb | 8 ++++++++ app/views/dashboard/tasks/index.html.haml | 4 ++-- app/views/layouts/header/_default.html.haml | 6 +++++- 5 files changed, 25 insertions(+), 6 deletions(-) diff --git a/app/assets/stylesheets/pages/tasks.scss b/app/assets/stylesheets/pages/tasks.scss index 702d7c55e59..f0e13f4d786 100644 --- a/app/assets/stylesheets/pages/tasks.scss +++ b/app/assets/stylesheets/pages/tasks.scss @@ -2,6 +2,16 @@ * Dashboard tasks queue * */ + +.navbar-nav { + li { + .badge.tasks-pending-count { + background-color: #7f8fa4; + margin-top: -5px; + } + } +} + .tasks { .panel { border-top: none; diff --git a/app/controllers/dashboard/tasks_controller.rb b/app/controllers/dashboard/tasks_controller.rb index 1102745067f..2f049da661c 100644 --- a/app/controllers/dashboard/tasks_controller.rb +++ b/app/controllers/dashboard/tasks_controller.rb @@ -10,9 +10,6 @@ class Dashboard::TasksController < Dashboard::ApplicationController end @tasks = @tasks.page(params[:page]).per(PER_PAGE) - - @pending_count = current_user.tasks.pending.count - @done_count = current_user.tasks.done.count end def destroy diff --git a/app/helpers/tasks_helper.rb b/app/helpers/tasks_helper.rb index 4a87f8d4ca4..cf4eab0ef94 100644 --- a/app/helpers/tasks_helper.rb +++ b/app/helpers/tasks_helper.rb @@ -9,6 +9,14 @@ module TasksHelper end end + def tasks_pending_count + current_user.tasks.pending.count + end + + def tasks_done_count + current_user.tasks.done.count + end + def task_action_name(task) target = task.target_type.titleize.downcase diff --git a/app/views/dashboard/tasks/index.html.haml b/app/views/dashboard/tasks/index.html.haml index 8147d017c3f..2f582009288 100644 --- a/app/views/dashboard/tasks/index.html.haml +++ b/app/views/dashboard/tasks/index.html.haml @@ -5,10 +5,10 @@ %ul.nav-links %li{class: ("active" if params[:state].blank? || params[:state] == 'pending')} = link_to dashboard_tasks_path(state: 'pending') do - Tasks (#{@pending_count}) + Tasks (#{tasks_pending_count}) %li{class: ("active" if params[:state] == 'done')} = link_to dashboard_tasks_path(state: 'done') do - Done (#{@done_count}) + Done (#{tasks_done_count}) .tasks - if @tasks.any? diff --git a/app/views/layouts/header/_default.html.haml b/app/views/layouts/header/_default.html.haml index fcb6b835a7e..d3aec9ae202 100644 --- a/app/views/layouts/header/_default.html.haml +++ b/app/views/layouts/header/_default.html.haml @@ -21,6 +21,10 @@ %li = link_to admin_root_path, title: 'Admin Area', data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do = icon('wrench fw') + %li + = link_to dashboard_tasks_path, title: 'Task Queue', data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do + %span.badge.tasks-pending-count + = tasks_pending_count - if current_user.can_create_project? %li = link_to new_project_path, title: 'New project', data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do @@ -39,4 +43,4 @@ = render 'shared/outdated_browser' - if @project && !@project.empty_repo? :javascript - var findFileURL = "#{namespace_project_find_file_path(@project.namespace, @project, @ref || @project.repository.root_ref)}"; \ No newline at end of file + var findFileURL = "#{namespace_project_find_file_path(@project.namespace, @project, @ref || @project.repository.root_ref)}"; -- 2.18.1 From b05ab108f8f054e137d6ca3df9c211152afa23c7 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 15 Feb 2016 20:26:30 -0200 Subject: [PATCH 08/50] Add feature spec for task queue list page --- features/dashboard/task_queue.feature | 15 +++++++ features/steps/dashboard/task_queue.rb | 60 ++++++++++++++++++++++++++ features/steps/shared/paths.rb | 4 ++ spec/factories/tasks.rb | 23 ++++++++++ 4 files changed, 102 insertions(+) create mode 100644 features/dashboard/task_queue.feature create mode 100644 features/steps/dashboard/task_queue.rb create mode 100644 spec/factories/tasks.rb diff --git a/features/dashboard/task_queue.feature b/features/dashboard/task_queue.feature new file mode 100644 index 00000000000..42b4a86e498 --- /dev/null +++ b/features/dashboard/task_queue.feature @@ -0,0 +1,15 @@ +@dashboard +Feature: Dashboard Task Queue + Background: + Given I sign in as a user + And I own project "Shop" + And "John Doe" is a developer of project "Shop" + And I have pending tasks + And I visit dashboard task queue page + + @javascript + Scenario: I mark pending tasks as done + Then I should see pending tasks assigned to me + And I mark the pending task as done + And I click on the "Done" tab + Then I should see all tasks marked as done diff --git a/features/steps/dashboard/task_queue.rb b/features/steps/dashboard/task_queue.rb new file mode 100644 index 00000000000..8695dd5cfb1 --- /dev/null +++ b/features/steps/dashboard/task_queue.rb @@ -0,0 +1,60 @@ +class Spinach::Features::DashboardTaskQueue < Spinach::FeatureSteps + include SharedAuthentication + include SharedPaths + include SharedProject + include SharedUser + + step '"John Doe" is a developer of project "Shop"' do + project.team << [john_doe, :developer] + end + + step 'I have pending tasks' do + create(:task, user: current_user, project: project, author: john_doe, target: assigned_issue, action: Task::ASSIGNED) + end + + step 'I should see pending tasks assigned to me' do + expect(page).to have_link 'Tasks (1)' + expect(page).to have_link 'Done (0)' + + page.within('.tasks') do + expect(page).to have_content project.name_with_namespace + expect(page).to have_content "John Doe assigned issue ##{assigned_issue.iid}" + expect(page).to have_content(assigned_issue.title[0..10]) + expect(page).to have_link 'Done' + end + end + + step 'I mark the pending task as done' do + click_link 'Done' + + expect(page).to have_content 'Task was successfully marked as done.' + expect(page).to have_link 'Tasks (0)' + expect(page).to have_link 'Done (1)' + expect(page).to have_content 'No tasks to show' + end + + step 'I click on the "Done" tab' do + click_link 'Done (1)' + end + + step 'I should see all tasks marked as done' do + page.within('.tasks') do + expect(page).to have_content project.name_with_namespace + expect(page).to have_content "John Doe assigned issue ##{assigned_issue.iid}" + expect(page).to have_content(assigned_issue.title[0..10]) + expect(page).not_to have_link 'Done' + end + end + + def assigned_issue + @assigned_issue ||= create(:issue, assignee: current_user, project: project) + end + + def john_doe + @john_doe ||= user_exists("John Doe", { username: "john_doe" }) + end + + def project + @project ||= create(:project, name: "Shop") + end +end diff --git a/features/steps/shared/paths.rb b/features/steps/shared/paths.rb index 2c854ac7bf9..112ace342f0 100644 --- a/features/steps/shared/paths.rb +++ b/features/steps/shared/paths.rb @@ -103,6 +103,10 @@ module SharedPaths visit dashboard_groups_path end + step 'I visit dashboard task queue page' do + visit dashboard_tasks_path + end + step 'I should be redirected to the dashboard groups page' do expect(current_path).to eq dashboard_groups_path end diff --git a/spec/factories/tasks.rb b/spec/factories/tasks.rb new file mode 100644 index 00000000000..cd04405774a --- /dev/null +++ b/spec/factories/tasks.rb @@ -0,0 +1,23 @@ +# == Schema Information +# +# Table name: tasks +# +# id :integer not null, primary key +# user_id :integer not null +# project_id :integer not null +# target_id :integer not null +# target_type :string not null +# author_id :integer +# action :integer +# state :string not null +# created_at :datetime +# updated_at :datetime +# + +FactoryGirl.define do + factory :task do + project + author + user + end +end -- 2.18.1 From 48ddf9a407e0f98949301de7a2cbd7d62f8a4e47 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 16 Feb 2016 11:14:30 -0200 Subject: [PATCH 09/50] Remove task abilities, since we will only ever show the user their own --- app/controllers/dashboard/tasks_controller.rb | 8 -------- app/models/ability.rb | 11 ----------- 2 files changed, 19 deletions(-) diff --git a/app/controllers/dashboard/tasks_controller.rb b/app/controllers/dashboard/tasks_controller.rb index 2f049da661c..d5f835babdc 100644 --- a/app/controllers/dashboard/tasks_controller.rb +++ b/app/controllers/dashboard/tasks_controller.rb @@ -1,6 +1,4 @@ class Dashboard::TasksController < Dashboard::ApplicationController - before_action :authorize_destroy_task!, only: [:destroy] - def index @tasks = case params[:state] when 'done' @@ -23,12 +21,6 @@ class Dashboard::TasksController < Dashboard::ApplicationController private - def authorize_destroy_task! - unless can?(current_user, :destroy_task, task) - return render_404 - end - end - def task @task ||= current_user.tasks.find(params[:id]) end diff --git a/app/models/ability.rb b/app/models/ability.rb index 5e0c76004d2..a866eadeebb 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -17,7 +17,6 @@ class Ability when Namespace then namespace_abilities(user, subject) when GroupMember then group_member_abilities(user, subject) when ProjectMember then project_member_abilities(user, subject) - when Task then task_abilities(user, subject) else [] end.concat(global_abilities(user)) end @@ -417,16 +416,6 @@ class Ability rules end - def task_abilities(user, task) - rules = [] - - if task && task.user == user - rules << :destroy_task - end - - rules - end - def abilities @abilities ||= begin abilities = Six.new -- 2.18.1 From e81061a211fedf9adaca8b053b6633988fdd5644 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 16 Feb 2016 00:26:33 -0200 Subject: [PATCH 10/50] Marks pending tasks for an user as done when he edit an issue --- app/services/issuable_base_service.rb | 2 +- app/services/issues/update_service.rb | 23 +++++- app/services/merge_requests/update_service.rb | 2 +- app/services/task_service.rb | 13 ++++ spec/factories/tasks.rb | 10 +++ spec/services/issues/update_service_spec.rb | 77 ++++++++++++++++--- spec/services/task_service_spec.rb | 17 ++++ 7 files changed, 130 insertions(+), 14 deletions(-) diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 2556f06e2d3..63025c10fb0 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -54,7 +54,7 @@ class IssuableBaseService < BaseService if params.present? && issuable.update_attributes(params.merge(updated_by: current_user)) issuable.reset_events_cache handle_common_system_notes(issuable, old_labels: old_labels) - handle_changes(issuable) + handle_changes(issuable, old_labels: old_labels) issuable.create_new_cross_references!(current_user) execute_hooks(issuable, 'update') end diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index e6afcb91652..fb9571247cc 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -4,7 +4,11 @@ module Issues update(issue) end - def handle_changes(issue) + def handle_changes(issue, options = {}) + if have_changes?(issue, options) + task_service.mark_as_done(issue, current_user) + end + if issue.previous_changes.include?('milestone_id') create_milestone_note(issue) end @@ -23,5 +27,22 @@ module Issues def close_service Issues::CloseService end + + private + + def have_changes?(issue, options = {}) + valid_attrs = [:title, :description, :assignee_id, :milestone_id] + + attrs_changed = valid_attrs.any? do |attr| + issue.previous_changes.include?(attr.to_s) + end + + old_labels = options[:old_labels] + labels_changed = old_labels && issue.labels != old_labels + + if attrs_changed || labels_changed + task_service.mark_as_done(issue, current_user) + end + end end end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 5ff2cc03dda..55322bf47bb 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -14,7 +14,7 @@ module MergeRequests update(merge_request) end - def handle_changes(merge_request) + def handle_changes(merge_request, options = {}) if merge_request.previous_changes.include?('target_branch') create_branch_change_note(merge_request, 'target', merge_request.previous_changes['target_branch'].first, diff --git a/app/services/task_service.rb b/app/services/task_service.rb index 35d258a70c2..2c312331ae9 100644 --- a/app/services/task_service.rb +++ b/app/services/task_service.rb @@ -26,6 +26,15 @@ class TaskService end end + # When we mark a task as done we should: + # + # * mark all pending tasks related to the target for the user as done + # + def mark_as_done(target, user) + pending_tasks = pending_tasks_for(user, target.project, target) + pending_tasks.update_all(state: :done) + end + private def create_task(project, target, author, user, action) @@ -40,4 +49,8 @@ class TaskService Task.create(attributes) end + + def pending_tasks_for(user, project, target) + user.tasks.pending.where(project: project, target: target) + end end diff --git a/spec/factories/tasks.rb b/spec/factories/tasks.rb index cd04405774a..710b8324f1f 100644 --- a/spec/factories/tasks.rb +++ b/spec/factories/tasks.rb @@ -19,5 +19,15 @@ FactoryGirl.define do project author user + + factory :pending_assigned_task, traits: [:assgined, :pending] + + trait :assgined do + action { Task::ASSIGNED } + end + + trait :pending do + state { :pending } + end end end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 8f654517bce..1b7d5b45168 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -78,18 +78,74 @@ describe Issues::UpdateService, services: true do expect(note).not_to be_nil expect(note.note).to eq 'Title changed from **Old title** to **New title**' end + end - it 'creates a pending task if being reassigned' do - attributes = { - project: project, - author: user, - user: user2, - target: issue, - action: Task::ASSIGNED, - state: :pending - } + context 'task queue' do + let!(:pending_task) do + create(:pending_assigned_task, user: user, project: project, target: issue, author: user2) + end + + context 'when the title change' do + before do + update_issue({ title: 'New title' }) + end + + it 'marks pending tasks as done' do + expect(pending_task.reload.done?).to eq true + end + end + + context 'when the description change' do + before do + update_issue({ description: 'Also please fix' }) + end + + it 'marks pending tasks as done' do + expect(pending_task.reload.done?).to eq true + end + end + + context 'when is reassigned' do + before do + update_issue({ assignee: user2 }) + end + + it 'marks previous assignee pending tasks as done' do + expect(pending_task.reload.done?).to eq true + end - expect(Task.where(attributes).count).to eq 1 + it 'creates a pending task for new assignee' do + attributes = { + project: project, + author: user, + user: user2, + target: issue, + action: Task::ASSIGNED, + state: :pending + } + + expect(Task.where(attributes).count).to eq 1 + end + end + + context 'when the milestone change' do + before do + update_issue({ milestone: create(:milestone) }) + end + + it 'marks pending tasks as done' do + expect(pending_task.reload.done?).to eq true + end + end + + context 'when the labels change' do + before do + update_issue({ label_ids: [label.id] }) + end + + it 'marks pending tasks as done' do + expect(pending_task.reload.done?).to eq true + end end end @@ -157,6 +213,5 @@ describe Issues::UpdateService, services: true do end end end - end end diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb index db9f77fd12d..e8920c59fee 100644 --- a/spec/services/task_service_spec.rb +++ b/spec/services/task_service_spec.rb @@ -45,6 +45,23 @@ describe TaskService, services: true do is_expected_to_not_create_task { service.reassigned_issue(assigned_issue, author) } end end + + describe '#mark_as_done' do + let!(:first_pending_task) do + create(:pending_assigned_task, user: john_doe, project: project, target: assigned_issue, author: author) + end + + let!(:second_pending_task) do + create(:pending_assigned_task, user: john_doe, project: project, target: assigned_issue, author: author) + end + + it 'marks related pending tasks to the target for the user as done' do + service.mark_as_done(assigned_issue, john_doe) + + expect(first_pending_task.reload.done?).to eq true + expect(second_pending_task.reload.done?).to eq true + end + end end def is_expected_to_create_pending_task(attributes = {}) -- 2.18.1 From a56ada0a1e92b9ce919fd75edb6508514d00148a Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 16 Feb 2016 18:31:40 -0200 Subject: [PATCH 11/50] Marks pending tasks for an user as done when he close the issue --- app/services/issues/close_service.rb | 2 ++ app/services/task_service.rb | 8 ++++++++ spec/services/issues/close_service_spec.rb | 8 ++++++++ spec/services/task_service_spec.rb | 17 +++++++++++++++++ 4 files changed, 35 insertions(+) diff --git a/app/services/issues/close_service.rb b/app/services/issues/close_service.rb index a1a20e47681..a652bba4761 100644 --- a/app/services/issues/close_service.rb +++ b/app/services/issues/close_service.rb @@ -3,6 +3,7 @@ module Issues def execute(issue, commit = nil) if project.jira_tracker? && project.jira_service.active project.jira_service.execute(commit, issue) + task_service.close_issue(issue, current_user) return issue end @@ -10,6 +11,7 @@ module Issues event_service.close_issue(issue, current_user) create_note(issue, commit) notification_service.close_issue(issue, current_user) + task_service.close_issue(issue, current_user) execute_hooks(issue, 'close') end diff --git a/app/services/task_service.rb b/app/services/task_service.rb index 2c312331ae9..0d66f89e997 100644 --- a/app/services/task_service.rb +++ b/app/services/task_service.rb @@ -16,6 +16,14 @@ class TaskService end end + # When close an issue we should: + # + # * mark all pending tasks related to the target for the current user as done + # + def close_issue(issue, current_user) + mark_as_done(issue, current_user) + end + # When we reassign an issue we should: # # * creates a pending task for new assignee if issue is assigned diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 3a8daf28f5e..7a587a96ddc 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -5,6 +5,9 @@ describe Issues::CloseService, services: true do let(:user2) { create(:user) } let(:issue) { create(:issue, assignee: user2) } let(:project) { issue.project } + let!(:pending_task) do + create(:pending_assigned_task, user: user, project: project, target: issue, author: user2) + end before do project.team << [user, :master] @@ -32,6 +35,10 @@ describe Issues::CloseService, services: true do note = @issue.notes.last expect(note.note).to include "Status changed to closed" end + + it 'marks pending tasks as done' do + expect(pending_task.reload).to be_done + end end context "external issue tracker" do @@ -42,6 +49,7 @@ describe Issues::CloseService, services: true do it { expect(@issue).to be_valid } it { expect(@issue).to be_opened } + it { expect(pending_task.reload).to be_pending } end end end diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb index e8920c59fee..04735900e25 100644 --- a/spec/services/task_service_spec.rb +++ b/spec/services/task_service_spec.rb @@ -31,6 +31,23 @@ describe TaskService, services: true do end end + describe '#close_issue' do + let!(:first_pending_task) do + create(:pending_assigned_task, user: john_doe, project: project, target: assigned_issue, author: author) + end + + let!(:second_pending_task) do + create(:pending_assigned_task, user: john_doe, project: project, target: assigned_issue, author: author) + end + + it 'marks related pending tasks to the target for the user as done' do + service.close_issue(assigned_issue, john_doe) + + expect(first_pending_task.reload.done?).to eq true + expect(second_pending_task.reload.done?).to eq true + end + end + describe '#reassigned_issue' do it 'creates a pending task for new assignee' do unassigned_issue.update_attribute(:assignee, john_doe) -- 2.18.1 From 917081fe9dd3da2995faf8f5555ed120ec4c4ea5 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 16 Feb 2016 18:42:46 -0200 Subject: [PATCH 12/50] Fix rubocop offenses --- app/controllers/dashboard/tasks_controller.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/controllers/dashboard/tasks_controller.rb b/app/controllers/dashboard/tasks_controller.rb index d5f835babdc..e3245609204 100644 --- a/app/controllers/dashboard/tasks_controller.rb +++ b/app/controllers/dashboard/tasks_controller.rb @@ -1,11 +1,11 @@ class Dashboard::TasksController < Dashboard::ApplicationController def index @tasks = case params[:state] - when 'done' - current_user.tasks.done - else - current_user.tasks.pending - end + when 'done' + current_user.tasks.done + else + current_user.tasks.pending + end @tasks = @tasks.page(params[:page]).per(PER_PAGE) end -- 2.18.1 From 49cd19652ade0eb81126caa590461e8340d63d26 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 16 Feb 2016 19:29:11 -0200 Subject: [PATCH 13/50] Mark pending tasks for the note author as done when he left a note --- app/services/notes/post_process_service.rb | 4 +- app/services/task_service.rb | 11 +++++ .../notes/post_process_service_spec.rb | 1 + spec/services/task_service_spec.rb | 43 +++++++++++++++++-- 4 files changed, 53 insertions(+), 6 deletions(-) diff --git a/app/services/notes/post_process_service.rb b/app/services/notes/post_process_service.rb index f37d3c50cdd..465b246cabc 100644 --- a/app/services/notes/post_process_service.rb +++ b/app/services/notes/post_process_service.rb @@ -1,6 +1,5 @@ module Notes class PostProcessService - attr_accessor :note def initialize(note) @@ -14,6 +13,8 @@ module Notes @note.create_cross_references! execute_note_hooks end + + TaskService.new.new_note(note) end def hook_data @@ -25,6 +26,5 @@ module Notes @note.project.execute_hooks(note_data, :note_hooks) @note.project.execute_services(note_data, :note_hooks) end - end end diff --git a/app/services/task_service.rb b/app/services/task_service.rb index 0d66f89e997..e58974a9b38 100644 --- a/app/services/task_service.rb +++ b/app/services/task_service.rb @@ -43,6 +43,17 @@ class TaskService pending_tasks.update_all(state: :done) end + # When create a note we should: + # + # * mark all pending tasks related to the noteable for the note author as done + # + def new_note(note) + # Skip system notes, like status changes and cross-references + unless note.system + mark_as_done(note.noteable, note.author) + end + end + private def create_task(project, target, author, user, action) diff --git a/spec/services/notes/post_process_service_spec.rb b/spec/services/notes/post_process_service_spec.rb index 1a3f339bd64..9489b8c6336 100644 --- a/spec/services/notes/post_process_service_spec.rb +++ b/spec/services/notes/post_process_service_spec.rb @@ -20,6 +20,7 @@ describe Notes::PostProcessService, services: true do it do expect(project).to receive(:execute_hooks) expect(project).to receive(:execute_services) + expect_any_instance_of(TaskService).to receive(:new_note).with(@note) Notes::PostProcessService.new(@note).execute end end diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb index 04735900e25..2fd75d10b6c 100644 --- a/spec/services/task_service_spec.rb +++ b/spec/services/task_service_spec.rb @@ -43,8 +43,8 @@ describe TaskService, services: true do it 'marks related pending tasks to the target for the user as done' do service.close_issue(assigned_issue, john_doe) - expect(first_pending_task.reload.done?).to eq true - expect(second_pending_task.reload.done?).to eq true + expect(first_pending_task.reload).to be_done + expect(second_pending_task.reload).to be_done end end @@ -75,8 +75,43 @@ describe TaskService, services: true do it 'marks related pending tasks to the target for the user as done' do service.mark_as_done(assigned_issue, john_doe) - expect(first_pending_task.reload.done?).to eq true - expect(second_pending_task.reload.done?).to eq true + expect(first_pending_task.reload).to be_done + expect(second_pending_task.reload).to be_done + end + end + + describe '#new_note' do + let!(:first_pending_task) do + create(:pending_assigned_task, user: john_doe, project: project, target: assigned_issue, author: author) + end + + let!(:second_pending_task) do + create(:pending_assigned_task, user: john_doe, project: project, target: assigned_issue, author: author) + end + + let(:note) { create(:note, project: project, noteable: assigned_issue, author: john_doe) } + let(:award_note) { create(:note, :award, project: project, noteable: assigned_issue, author: john_doe, note: 'thumbsup') } + let(:system_note) { create(:system_note, project: project, noteable: assigned_issue) } + + it 'mark related pending tasks to the noteable for the note author as done' do + service.new_note(note) + + expect(first_pending_task.reload).to be_done + expect(second_pending_task.reload).to be_done + end + + it 'mark related pending tasks to the noteable for the award note author as done' do + service.new_note(award_note) + + expect(first_pending_task.reload).to be_done + expect(second_pending_task.reload).to be_done + end + + it 'does not mark related pending tasks it is a system note' do + service.new_note(system_note) + + expect(first_pending_task.reload).to be_pending + expect(second_pending_task.reload).to be_pending end end end -- 2.18.1 From 9da03c45c9e313e8adb170ee9933bd94efbf747c Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 16 Feb 2016 20:21:24 -0200 Subject: [PATCH 14/50] Mark pending tasks for the current user as done when he edit a note --- app/services/notes/update_service.rb | 4 ++ app/services/task_service.rb | 11 ++++++ spec/services/notes/update_service_spec.rb | 45 ++++++++++++++++++++++ 3 files changed, 60 insertions(+) create mode 100644 spec/services/notes/update_service_spec.rb diff --git a/app/services/notes/update_service.rb b/app/services/notes/update_service.rb index 72e2f78008d..6c2d36546e1 100644 --- a/app/services/notes/update_service.rb +++ b/app/services/notes/update_service.rb @@ -7,6 +7,10 @@ module Notes note.create_new_cross_references!(current_user) note.reset_events_cache + if note.previous_changes.include?('note') + TaskService.new.update_note(note, current_user) + end + note end end diff --git a/app/services/task_service.rb b/app/services/task_service.rb index e58974a9b38..57f68c61d00 100644 --- a/app/services/task_service.rb +++ b/app/services/task_service.rb @@ -54,6 +54,17 @@ class TaskService end end + # When update a note we should: + # + # * mark all pending tasks related to the noteable for the current user as done + # + def update_note(note, current_user) + # Skip system notes, like status changes and cross-references + unless note.system + mark_as_done(note.noteable, current_user) + end + end + private def create_task(project, target, author, user, action) diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb new file mode 100644 index 00000000000..e6670143951 --- /dev/null +++ b/spec/services/notes/update_service_spec.rb @@ -0,0 +1,45 @@ +require 'spec_helper' + +describe Notes::UpdateService, services: true do + let(:project) { create(:empty_project) } + let(:user) { create(:user) } + let(:user2) { create(:user) } + let(:issue) { create(:issue, project: project) } + let(:note) { create(:note, project: project, noteable: issue, author: user, note: 'Old note') } + + before do + project.team << [user, :master] + project.team << [user2, :developer] + end + + describe '#execute' do + def update_note(opts) + @note = Notes::UpdateService.new(project, user, opts).execute(note) + @note.reload + end + + context 'task queue' do + let!(:task) { create(:pending_assigned_task, user: user, project: project, target: issue, author: user2) } + + context 'when the note change' do + before do + update_note({ note: 'New note' }) + end + + it 'marks pending tasks as done' do + expect(task.reload).to be_done + end + end + + context 'when the note does not change' do + before do + update_note({ note: 'Old note' }) + end + + it 'keep pending tasks' do + expect(task.reload).to be_pending + end + end + end + end +end -- 2.18.1 From 802bf6d012bf8edb8ea35429bd8b8cc4fe8e9bf1 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 16 Feb 2016 21:01:14 -0200 Subject: [PATCH 15/50] Create a pending task when an MR is assigned to someone --- app/services/merge_requests/create_service.rb | 3 +- app/services/merge_requests/update_service.rb | 1 + app/services/task_service.rb | 36 ++++++++++--- .../merge_requests/create_service_spec.rb | 40 ++++++++++++++ .../merge_requests/update_service_spec.rb | 25 +++++++++ spec/services/task_service_spec.rb | 52 ++++++++++++++++--- 6 files changed, 142 insertions(+), 15 deletions(-) diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 009d5a6867e..b5691cdf44f 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -2,7 +2,7 @@ module MergeRequests class CreateService < MergeRequests::BaseService def execute # @project is used to determine whether the user can set the merge request's - # assignee, milestone and labels. Whether they can depends on their + # assignee, milestone and labels. Whether they can depends on their # permissions on the target project. source_project = @project @project = Project.find(params[:target_project_id]) if params[:target_project_id] @@ -18,6 +18,7 @@ module MergeRequests merge_request.update_attributes(label_ids: label_params) event_service.open_mr(merge_request, current_user) notification_service.new_merge_request(merge_request, current_user) + task_service.new_merge_request(merge_request, current_user) merge_request.create_cross_references!(current_user) execute_hooks(merge_request) end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 55322bf47bb..85b11def231 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -28,6 +28,7 @@ module MergeRequests if merge_request.previous_changes.include?('assignee_id') create_assignee_note(merge_request) notification_service.reassigned_merge_request(merge_request, current_user) + task_service.reassigned_merge_request(merge_request, current_user) end if merge_request.previous_changes.include?('target_branch') || diff --git a/app/services/task_service.rb b/app/services/task_service.rb index 57f68c61d00..329a041061a 100644 --- a/app/services/task_service.rb +++ b/app/services/task_service.rb @@ -11,9 +11,7 @@ class TaskService # * creates a pending task for assignee if issue is assigned # def new_issue(issue, current_user) - if issue.is_assigned? && issue.assignee != current_user - create_task(issue.project, issue, current_user, issue.assignee, Task::ASSIGNED) - end + new_issuable(issue, current_user) end # When close an issue we should: @@ -29,9 +27,23 @@ class TaskService # * creates a pending task for new assignee if issue is assigned # def reassigned_issue(issue, current_user) - if issue.is_assigned? - create_task(issue.project, issue, current_user, issue.assignee, Task::ASSIGNED) - end + reassigned_issuable(issue, current_user) + end + + # When create a merge request we should: + # + # * creates a pending task for assignee if merge request is assigned + # + def new_merge_request(merge_request, current_user) + new_issuable(merge_request, current_user) + end + + # When we reassign an merge request we should: + # + # * creates a pending task for new assignee if merge request is assigned + # + def reassigned_merge_request(merge_request, current_user) + reassigned_issuable(merge_request, current_user) end # When we mark a task as done we should: @@ -83,4 +95,16 @@ class TaskService def pending_tasks_for(user, project, target) user.tasks.pending.where(project: project, target: target) end + + def new_issuable(issuable, user) + if issuable.is_assigned? && issuable.assignee != user + create_task(issuable.project, issuable, user, issuable.assignee, Task::ASSIGNED) + end + end + + def reassigned_issuable(issuable, user) + if issuable.is_assigned? + create_task(issuable.project, issuable, user, issuable.assignee, Task::ASSIGNED) + end + end end diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index be8f1676eeb..2e3c9f88369 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' describe MergeRequests::CreateService, services: true do let(:project) { create(:project) } let(:user) { create(:user) } + let(:assignee) { create(:user) } describe :execute do context 'valid params' do @@ -14,10 +15,12 @@ describe MergeRequests::CreateService, services: true do target_branch: 'master' } end + let(:service) { MergeRequests::CreateService.new(project, user, opts) } before do project.team << [user, :master] + project.team << [assignee, :developer] allow(service).to receive(:execute_hooks) @merge_request = service.execute @@ -25,10 +28,47 @@ describe MergeRequests::CreateService, services: true do it { expect(@merge_request).to be_valid } it { expect(@merge_request.title).to eq('Awesome merge_request') } + it { expect(@merge_request.assignee).to be_nil } it 'should execute hooks with default action' do expect(service).to have_received(:execute_hooks).with(@merge_request) end + + it 'does not creates a pending task' do + attributes = { + project: project, + target: @merge_request + } + + expect(Task.where(attributes).count).to be_zero + end + + context 'when merge request is assigned to someone' do + let(:opts) do + { + title: 'Awesome merge_request', + description: 'please fix', + source_branch: 'feature', + target_branch: 'master', + assignee: assignee + } + end + + it { expect(@merge_request.assignee).to eq assignee } + + it 'creates a pending task for new assignee' do + attributes = { + project: project, + author: user, + user: assignee, + target: @merge_request, + action: Task::ASSIGNED, + state: :pending + } + + expect(Task.where(attributes).count).to eq 1 + end + end end end end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 2e9e6e0870d..88853483784 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -98,6 +98,31 @@ describe MergeRequests::UpdateService, services: true do end end + context 'task queue' do + let!(:pending_task) do + create(:pending_assigned_task, user: user, project: project, target: merge_request, author: user2) + end + + context 'when is reassigned' do + before do + update_merge_request({ assignee: user2 }) + end + + it 'creates a pending task for new assignee' do + attributes = { + project: project, + author: user, + user: user2, + target: merge_request, + action: Task::ASSIGNED, + state: :pending + } + + expect(Task.where(attributes).count).to eq 1 + end + end + end + context 'when MergeRequest has tasks' do before { update_merge_request({ description: "- [ ] Task 1\n- [ ] Task 2" }) } diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb index 2fd75d10b6c..0a072dda94b 100644 --- a/spec/services/task_service_spec.rb +++ b/spec/services/task_service_spec.rb @@ -1,20 +1,20 @@ require 'spec_helper' describe TaskService, services: true do + let(:author) { create(:user) } + let(:john_doe) { create(:user) } + let(:project) { create(:project) } let(:service) { described_class.new } + before do + project.team << [author, :developer] + project.team << [john_doe, :developer] + end + describe 'Issues' do - let(:author) { create(:user) } - let(:john_doe) { create(:user) } - let(:project) { create(:empty_project, :public) } let(:assigned_issue) { create(:issue, project: project, assignee: john_doe) } let(:unassigned_issue) { create(:issue, project: project, assignee: nil) } - before do - project.team << [author, :developer] - project.team << [john_doe, :developer] - end - describe '#new_issue' do it 'creates a pending task if assigned' do service.new_issue(assigned_issue, author) @@ -116,6 +116,42 @@ describe TaskService, services: true do end end + describe 'Merge Requests' do + let(:mr_assigned) { create(:merge_request, source_project: project, assignee: john_doe) } + let(:mr_unassigned) { create(:merge_request, source_project: project, assignee: nil) } + + describe '#new_merge_request' do + it 'creates a pending task if assigned' do + service.new_merge_request(mr_assigned, author) + + is_expected_to_create_pending_task(user: john_doe, target: mr_assigned, action: Task::ASSIGNED) + end + + it 'does not create a task if unassigned' do + is_expected_to_not_create_task { service.new_merge_request(mr_unassigned, author) } + end + + it 'does not create a task if assignee is the current user' do + is_expected_to_not_create_task { service.new_merge_request(mr_unassigned, john_doe) } + end + end + + describe '#reassigned_merge_request' do + it 'creates a pending task for new assignee' do + mr_unassigned.update_attribute(:assignee, john_doe) + service.reassigned_merge_request(mr_unassigned, author) + + is_expected_to_create_pending_task(user: john_doe, target: mr_unassigned, action: Task::ASSIGNED) + end + + it 'does not create a task if unassigned' do + mr_assigned.update_attribute(:assignee, nil) + + is_expected_to_not_create_task { service.reassigned_merge_request(mr_assigned, author) } + end + end + end + def is_expected_to_create_pending_task(attributes = {}) attributes.reverse_merge!( project: project, -- 2.18.1 From 7bed6ee21d1615b4625754f3af803496938ecef6 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 16 Feb 2016 21:18:38 -0200 Subject: [PATCH 16/50] Marks pending tasks for an user as done when he edit a MR --- app/services/merge_requests/update_service.rb | 21 +++++++ .../merge_requests/update_service_spec.rb | 55 ++++++++++++++++++- 2 files changed, 75 insertions(+), 1 deletion(-) diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 85b11def231..34ee6b3eecd 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -15,6 +15,10 @@ module MergeRequests end def handle_changes(merge_request, options = {}) + if have_changes?(merge_request, options) + task_service.mark_as_done(merge_request, current_user) + end + if merge_request.previous_changes.include?('target_branch') create_branch_change_note(merge_request, 'target', merge_request.previous_changes['target_branch'].first, @@ -44,5 +48,22 @@ module MergeRequests def close_service MergeRequests::CloseService end + + private + + def have_changes?(merge_request, options = {}) + valid_attrs = [:title, :description, :assignee_id, :milestone_id, :target_branch] + + attrs_changed = valid_attrs.any? do |attr| + merge_request.previous_changes.include?(attr.to_s) + end + + old_labels = options[:old_labels] + labels_changed = old_labels && merge_request.labels != old_labels + + if attrs_changed || labels_changed + task_service.mark_as_done(merge_request, current_user) + end + end end end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 88853483784..77c661fd293 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -103,11 +103,35 @@ describe MergeRequests::UpdateService, services: true do create(:pending_assigned_task, user: user, project: project, target: merge_request, author: user2) end + context 'when the title change' do + before do + update_merge_request({ title: 'New title' }) + end + + it 'marks pending tasks as done' do + expect(pending_task.reload).to be_done + end + end + + context 'when the description change' do + before do + update_merge_request({ description: 'Also please fix' }) + end + + it 'marks pending tasks as done' do + expect(pending_task.reload).to be_done + end + end + context 'when is reassigned' do before do update_merge_request({ assignee: user2 }) end + it 'marks previous assignee pending tasks as done' do + expect(pending_task.reload).to be_done + end + it 'creates a pending task for new assignee' do attributes = { project: project, @@ -121,6 +145,36 @@ describe MergeRequests::UpdateService, services: true do expect(Task.where(attributes).count).to eq 1 end end + + context 'when the milestone change' do + before do + update_merge_request({ milestone: create(:milestone) }) + end + + it 'marks pending tasks as done' do + expect(pending_task.reload).to be_done + end + end + + context 'when the labels change' do + before do + update_merge_request({ label_ids: [label.id] }) + end + + it 'marks pending tasks as done' do + expect(pending_task.reload).to be_done + end + end + + context 'when the target branch change' do + before do + update_merge_request({ target_branch: 'target' }) + end + + it 'marks pending tasks as done' do + expect(pending_task.reload).to be_done + end + end end context 'when MergeRequest has tasks' do @@ -155,6 +209,5 @@ describe MergeRequests::UpdateService, services: true do end end end - end end -- 2.18.1 From c86b12bf69828da47dc535021aa3847d61d2d642 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 16 Feb 2016 21:47:33 -0200 Subject: [PATCH 17/50] Marks pending tasks for an user as done when he close the MR --- app/services/merge_requests/close_service.rb | 1 + app/services/task_service.rb | 10 +++++++++- .../merge_requests/close_service_spec.rb | 7 +++++++ spec/services/task_service_spec.rb | 17 +++++++++++++++++ 4 files changed, 34 insertions(+), 1 deletion(-) diff --git a/app/services/merge_requests/close_service.rb b/app/services/merge_requests/close_service.rb index 47454f9f0c2..1f70c95ab4a 100644 --- a/app/services/merge_requests/close_service.rb +++ b/app/services/merge_requests/close_service.rb @@ -9,6 +9,7 @@ module MergeRequests event_service.close_mr(merge_request, current_user) create_note(merge_request) notification_service.close_mr(merge_request, current_user) + task_service.close_merge_request(merge_request, current_user) execute_hooks(merge_request, 'close') end diff --git a/app/services/task_service.rb b/app/services/task_service.rb index 329a041061a..8254f16581e 100644 --- a/app/services/task_service.rb +++ b/app/services/task_service.rb @@ -38,7 +38,15 @@ class TaskService new_issuable(merge_request, current_user) end - # When we reassign an merge request we should: + # When close a merge request we should: + # + # * mark all pending tasks related to the target for the current user as done + # + def close_merge_request(merge_request, current_user) + mark_as_done(merge_request, current_user) + end + + # When we reassign a merge request we should: # # * creates a pending task for new assignee if merge request is assigned # diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index 50d0c288790..3e72be2dc25 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -5,6 +5,9 @@ describe MergeRequests::CloseService, services: true do let(:user2) { create(:user) } let(:merge_request) { create(:merge_request, assignee: user2) } let(:project) { merge_request.project } + let!(:pending_task) do + create(:pending_assigned_task, user: user, project: project, target: merge_request, author: user2) + end before do project.team << [user, :master] @@ -41,6 +44,10 @@ describe MergeRequests::CloseService, services: true do note = @merge_request.notes.last expect(note.note).to include 'Status changed to closed' end + + it 'marks pending tasks as done' do + expect(pending_task.reload).to be_done + end end end end diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb index 0a072dda94b..124f1c659d0 100644 --- a/spec/services/task_service_spec.rb +++ b/spec/services/task_service_spec.rb @@ -136,6 +136,23 @@ describe TaskService, services: true do end end + describe '#close_merge_request' do + let!(:first_pending_task) do + create(:pending_assigned_task, user: john_doe, project: project, target: mr_assigned, author: author) + end + + let!(:second_pending_task) do + create(:pending_assigned_task, user: john_doe, project: project, target: mr_assigned, author: author) + end + + it 'marks related pending tasks to the target for the user as done' do + service.close_merge_request(mr_assigned, john_doe) + + expect(first_pending_task.reload).to be_done + expect(second_pending_task.reload).to be_done + end + end + describe '#reassigned_merge_request' do it 'creates a pending task for new assignee' do mr_unassigned.update_attribute(:assignee, john_doe) -- 2.18.1 From 372cdf4ce5fa124e03e2c93e9d1ed586d489b1d8 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 16 Feb 2016 23:54:10 -0200 Subject: [PATCH 18/50] Marks pending tasks for an user as done when he merge the MR --- .../merge_requests/post_merge_service.rb | 1 + app/services/task_service.rb | 8 ++++++++ spec/services/task_service_spec.rb | 17 +++++++++++++++++ 3 files changed, 26 insertions(+) diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb index 8f25c5e2496..63c2f1f4249 100644 --- a/app/services/merge_requests/post_merge_service.rb +++ b/app/services/merge_requests/post_merge_service.rb @@ -11,6 +11,7 @@ module MergeRequests create_merge_event(merge_request, current_user) create_note(merge_request) notification_service.merge_mr(merge_request, current_user) + task_service.merge_merge_request(merge_request, current_user) execute_hooks(merge_request, 'merge') end diff --git a/app/services/task_service.rb b/app/services/task_service.rb index 8254f16581e..2d65eee854d 100644 --- a/app/services/task_service.rb +++ b/app/services/task_service.rb @@ -54,6 +54,14 @@ class TaskService reassigned_issuable(merge_request, current_user) end + # When merge a merge request we should: + # + # * mark all pending tasks related to the target for the current user as done + # + def merge_merge_request(merge_request, current_user) + mark_as_done(merge_request, current_user) + end + # When we mark a task as done we should: # # * mark all pending tasks related to the target for the user as done diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb index 124f1c659d0..8e78f4e242a 100644 --- a/spec/services/task_service_spec.rb +++ b/spec/services/task_service_spec.rb @@ -167,6 +167,23 @@ describe TaskService, services: true do is_expected_to_not_create_task { service.reassigned_merge_request(mr_assigned, author) } end end + + describe '#merge_merge_request' do + let!(:first_pending_task) do + create(:pending_assigned_task, user: john_doe, project: project, target: mr_assigned, author: author) + end + + let!(:second_pending_task) do + create(:pending_assigned_task, user: john_doe, project: project, target: mr_assigned, author: author) + end + + it 'marks related pending tasks to the target for the user as done' do + service.merge_merge_request(mr_assigned, john_doe) + + expect(first_pending_task.reload).to be_done + expect(second_pending_task.reload).to be_done + end + end end def is_expected_to_create_pending_task(attributes = {}) -- 2.18.1 From 56a5fc0cd331143c9a2b4215db3dba6cefd6cc3e Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 17 Feb 2016 14:33:30 -0200 Subject: [PATCH 19/50] Move common behavior to to IssuableBaseService --- app/services/issuable_base_service.rb | 13 +++++++++++++ app/services/issues/update_service.rb | 17 ----------------- app/services/merge_requests/update_service.rb | 17 ----------------- 3 files changed, 13 insertions(+), 34 deletions(-) diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 63025c10fb0..fef96639ace 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -71,6 +71,19 @@ class IssuableBaseService < BaseService end end + def have_changes?(issuable, options = {}) + valid_attrs = [:title, :description, :assignee_id, :milestone_id, :target_branch] + + attrs_changed = valid_attrs.any? do |attr| + issuable.previous_changes.include?(attr.to_s) + end + + old_labels = options[:old_labels] + labels_changed = old_labels && issuable.labels != old_labels + + attrs_changed || labels_changed + end + def handle_common_system_notes(issuable, options = {}) if issuable.previous_changes.include?('title') create_title_change_note(issuable, issuable.previous_changes['title'].first) diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index fb9571247cc..48645a8e558 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -27,22 +27,5 @@ module Issues def close_service Issues::CloseService end - - private - - def have_changes?(issue, options = {}) - valid_attrs = [:title, :description, :assignee_id, :milestone_id] - - attrs_changed = valid_attrs.any? do |attr| - issue.previous_changes.include?(attr.to_s) - end - - old_labels = options[:old_labels] - labels_changed = old_labels && issue.labels != old_labels - - if attrs_changed || labels_changed - task_service.mark_as_done(issue, current_user) - end - end end end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 34ee6b3eecd..f9f4d9a4f27 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -48,22 +48,5 @@ module MergeRequests def close_service MergeRequests::CloseService end - - private - - def have_changes?(merge_request, options = {}) - valid_attrs = [:title, :description, :assignee_id, :milestone_id, :target_branch] - - attrs_changed = valid_attrs.any? do |attr| - merge_request.previous_changes.include?(attr.to_s) - end - - old_labels = options[:old_labels] - labels_changed = old_labels && merge_request.labels != old_labels - - if attrs_changed || labels_changed - task_service.mark_as_done(merge_request, current_user) - end - end end end -- 2.18.1 From 4bfc377ff62656879691c0a6fc03a4b2c534f42e Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 17 Feb 2016 14:36:37 -0200 Subject: [PATCH 20/50] Rename TaskService#mark_as_done to mark_pending_tasks_as_done --- app/services/issues/update_service.rb | 2 +- app/services/merge_requests/update_service.rb | 2 +- app/services/task_service.rb | 12 ++++++------ spec/services/task_service_spec.rb | 4 ++-- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index 48645a8e558..9e39847dc54 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -6,7 +6,7 @@ module Issues def handle_changes(issue, options = {}) if have_changes?(issue, options) - task_service.mark_as_done(issue, current_user) + task_service.mark_pending_tasks_as_done(issue, current_user) end if issue.previous_changes.include?('milestone_id') diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index f9f4d9a4f27..38ae067877a 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -16,7 +16,7 @@ module MergeRequests def handle_changes(merge_request, options = {}) if have_changes?(merge_request, options) - task_service.mark_as_done(merge_request, current_user) + task_service.mark_pending_tasks_as_done(merge_request, current_user) end if merge_request.previous_changes.include?('target_branch') diff --git a/app/services/task_service.rb b/app/services/task_service.rb index 2d65eee854d..4c040ee0fff 100644 --- a/app/services/task_service.rb +++ b/app/services/task_service.rb @@ -19,7 +19,7 @@ class TaskService # * mark all pending tasks related to the target for the current user as done # def close_issue(issue, current_user) - mark_as_done(issue, current_user) + mark_pending_tasks_as_done(issue, current_user) end # When we reassign an issue we should: @@ -43,7 +43,7 @@ class TaskService # * mark all pending tasks related to the target for the current user as done # def close_merge_request(merge_request, current_user) - mark_as_done(merge_request, current_user) + mark_pending_tasks_as_done(merge_request, current_user) end # When we reassign a merge request we should: @@ -59,14 +59,14 @@ class TaskService # * mark all pending tasks related to the target for the current user as done # def merge_merge_request(merge_request, current_user) - mark_as_done(merge_request, current_user) + mark_pending_tasks_as_done(merge_request, current_user) end # When we mark a task as done we should: # # * mark all pending tasks related to the target for the user as done # - def mark_as_done(target, user) + def mark_pending_tasks_as_done(target, user) pending_tasks = pending_tasks_for(user, target.project, target) pending_tasks.update_all(state: :done) end @@ -78,7 +78,7 @@ class TaskService def new_note(note) # Skip system notes, like status changes and cross-references unless note.system - mark_as_done(note.noteable, note.author) + mark_pending_tasks_as_done(note.noteable, note.author) end end @@ -89,7 +89,7 @@ class TaskService def update_note(note, current_user) # Skip system notes, like status changes and cross-references unless note.system - mark_as_done(note.noteable, current_user) + mark_pending_tasks_as_done(note.noteable, current_user) end end diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb index 8e78f4e242a..0cd0011b041 100644 --- a/spec/services/task_service_spec.rb +++ b/spec/services/task_service_spec.rb @@ -63,7 +63,7 @@ describe TaskService, services: true do end end - describe '#mark_as_done' do + describe '#mark_pending_tasks_as_done' do let!(:first_pending_task) do create(:pending_assigned_task, user: john_doe, project: project, target: assigned_issue, author: author) end @@ -73,7 +73,7 @@ describe TaskService, services: true do end it 'marks related pending tasks to the target for the user as done' do - service.mark_as_done(assigned_issue, john_doe) + service.mark_pending_tasks_as_done(assigned_issue, john_doe) expect(first_pending_task.reload).to be_done expect(second_pending_task.reload).to be_done -- 2.18.1 From 14fc05ebfdfb6654859ee6f57aa462420a6bcb56 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 17 Feb 2016 15:46:26 -0200 Subject: [PATCH 21/50] Create a pending task when a user is mentioned on issue/mr --- app/models/task.rb | 2 + app/services/task_service.rb | 32 ++++++++-- spec/services/task_service_spec.rb | 96 ++++++++++++++++++++---------- 3 files changed, 94 insertions(+), 36 deletions(-) diff --git a/app/models/task.rb b/app/models/task.rb index b9e5152b819..c9881991e38 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -16,6 +16,7 @@ class Task < ActiveRecord::Base ASSIGNED = 1 + MENTIONED = 2 belongs_to :author, class_name: "User" belongs_to :project @@ -43,6 +44,7 @@ class Task < ActiveRecord::Base def action_name case action when ASSIGNED then 'assigned' + when MENTIONED then 'mentioned on' end end diff --git a/app/services/task_service.rb b/app/services/task_service.rb index 4c040ee0fff..fa615f4cfcf 100644 --- a/app/services/task_service.rb +++ b/app/services/task_service.rb @@ -67,8 +67,7 @@ class TaskService # * mark all pending tasks related to the target for the user as done # def mark_pending_tasks_as_done(target, user) - pending_tasks = pending_tasks_for(user, target.project, target) - pending_tasks.update_all(state: :done) + pending_tasks(user, target.project, target).update_all(state: :done) end # When create a note we should: @@ -108,13 +107,34 @@ class TaskService Task.create(attributes) end - def pending_tasks_for(user, project, target) + def build_mentioned_users(project, target, author) + mentioned_users = target.mentioned_users.select do |user| + user.can?(:read_project, project) + end + + mentioned_users.delete(author) + mentioned_users.delete(target.assignee) if target.respond_to?(:assignee) + + mentioned_users.uniq + end + + def pending_tasks(user, project, target) user.tasks.pending.where(project: project, target: target) end - def new_issuable(issuable, user) - if issuable.is_assigned? && issuable.assignee != user - create_task(issuable.project, issuable, user, issuable.assignee, Task::ASSIGNED) + def new_issuable(issuable, current_user) + project = issuable.project + target = issuable + author = issuable.author + + if target.is_assigned? && target.assignee != current_user + create_task(project, target, author, target.assignee, Task::ASSIGNED) + end + + mentioned_users = build_mentioned_users(project, target, author) + + mentioned_users.each do |mentioned_user| + create_task(project, target, author, mentioned_user, Task::MENTIONED) end end diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb index 0cd0011b041..b35715c8b7a 100644 --- a/spec/services/task_service_spec.rb +++ b/spec/services/task_service_spec.rb @@ -2,46 +2,61 @@ require 'spec_helper' describe TaskService, services: true do let(:author) { create(:user) } - let(:john_doe) { create(:user) } + let(:john_doe) { create(:user, username: 'john_doe') } + let(:michael) { create(:user, username: 'michael') } + let(:stranger) { create(:user, username: 'stranger') } let(:project) { create(:project) } + let(:mentions) { [author.to_reference, john_doe.to_reference, michael.to_reference, stranger.to_reference].join(' ') } let(:service) { described_class.new } before do project.team << [author, :developer] project.team << [john_doe, :developer] + project.team << [michael, :developer] end describe 'Issues' do - let(:assigned_issue) { create(:issue, project: project, assignee: john_doe) } + let(:issue) { create(:issue, project: project, assignee: john_doe, author: author) } let(:unassigned_issue) { create(:issue, project: project, assignee: nil) } describe '#new_issue' do - it 'creates a pending task if assigned' do - service.new_issue(assigned_issue, author) + it 'creates a task if assigned' do + service.new_issue(issue, author) - is_expected_to_create_pending_task(user: john_doe, target: assigned_issue, action: Task::ASSIGNED) + should_create_task(user: john_doe, target: issue, action: Task::ASSIGNED) end it 'does not create a task if unassigned' do - is_expected_to_not_create_task { service.new_issue(unassigned_issue, author) } + should_not_create_any_task { service.new_issue(unassigned_issue, author) } end it 'does not create a task if assignee is the current user' do - is_expected_to_not_create_task { service.new_issue(unassigned_issue, john_doe) } + should_not_create_any_task { service.new_issue(unassigned_issue, john_doe) } + end + + it 'creates a task for each valid mentioned user' do + issue.update_attribute(:description, mentions) + + service.new_issue(issue, author) + + should_create_task(user: michael, target: issue, action: Task::MENTIONED) + should_not_create_task(user: author, target: issue, action: Task::MENTIONED) + should_not_create_task(user: john_doe, target: issue, action: Task::MENTIONED) + should_not_create_task(user: stranger, target: issue, action: Task::MENTIONED) end end describe '#close_issue' do let!(:first_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: assigned_issue, author: author) + create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author) end let!(:second_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: assigned_issue, author: author) + create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author) end it 'marks related pending tasks to the target for the user as done' do - service.close_issue(assigned_issue, john_doe) + service.close_issue(issue, john_doe) expect(first_pending_task.reload).to be_done expect(second_pending_task.reload).to be_done @@ -53,27 +68,27 @@ describe TaskService, services: true do unassigned_issue.update_attribute(:assignee, john_doe) service.reassigned_issue(unassigned_issue, author) - is_expected_to_create_pending_task(user: john_doe, target: unassigned_issue, action: Task::ASSIGNED) + should_create_task(user: john_doe, target: unassigned_issue, action: Task::ASSIGNED) end it 'does not create a task if unassigned' do - assigned_issue.update_attribute(:assignee, nil) + issue.update_attribute(:assignee, nil) - is_expected_to_not_create_task { service.reassigned_issue(assigned_issue, author) } + should_not_create_any_task { service.reassigned_issue(issue, author) } end end describe '#mark_pending_tasks_as_done' do let!(:first_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: assigned_issue, author: author) + create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author) end let!(:second_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: assigned_issue, author: author) + create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author) end it 'marks related pending tasks to the target for the user as done' do - service.mark_pending_tasks_as_done(assigned_issue, john_doe) + service.mark_pending_tasks_as_done(issue, john_doe) expect(first_pending_task.reload).to be_done expect(second_pending_task.reload).to be_done @@ -82,16 +97,16 @@ describe TaskService, services: true do describe '#new_note' do let!(:first_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: assigned_issue, author: author) + create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author) end let!(:second_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: assigned_issue, author: author) + create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author) end - let(:note) { create(:note, project: project, noteable: assigned_issue, author: john_doe) } - let(:award_note) { create(:note, :award, project: project, noteable: assigned_issue, author: john_doe, note: 'thumbsup') } - let(:system_note) { create(:system_note, project: project, noteable: assigned_issue) } + let(:note) { create(:note, project: project, noteable: issue, author: john_doe) } + let(:award_note) { create(:note, :award, project: project, noteable: issue, author: john_doe, note: 'thumbsup') } + let(:system_note) { create(:system_note, project: project, noteable: issue) } it 'mark related pending tasks to the noteable for the note author as done' do service.new_note(note) @@ -117,22 +132,33 @@ describe TaskService, services: true do end describe 'Merge Requests' do - let(:mr_assigned) { create(:merge_request, source_project: project, assignee: john_doe) } - let(:mr_unassigned) { create(:merge_request, source_project: project, assignee: nil) } + let(:mr_assigned) { create(:merge_request, source_project: project, author: author, assignee: john_doe) } + let(:mr_unassigned) { create(:merge_request, source_project: project, author: author, assignee: nil) } describe '#new_merge_request' do it 'creates a pending task if assigned' do service.new_merge_request(mr_assigned, author) - is_expected_to_create_pending_task(user: john_doe, target: mr_assigned, action: Task::ASSIGNED) + should_create_task(user: john_doe, target: mr_assigned, action: Task::ASSIGNED) end it 'does not create a task if unassigned' do - is_expected_to_not_create_task { service.new_merge_request(mr_unassigned, author) } + should_not_create_any_task { service.new_merge_request(mr_unassigned, author) } end it 'does not create a task if assignee is the current user' do - is_expected_to_not_create_task { service.new_merge_request(mr_unassigned, john_doe) } + should_not_create_any_task { service.new_merge_request(mr_unassigned, john_doe) } + end + + it 'creates a task for each valid mentioned user' do + mr_assigned.update_attribute(:description, mentions) + + service.new_merge_request(mr_assigned, author) + + should_create_task(user: michael, target: mr_assigned, action: Task::MENTIONED) + should_not_create_task(user: author, target: mr_assigned, action: Task::MENTIONED) + should_not_create_task(user: john_doe, target: mr_assigned, action: Task::MENTIONED) + should_not_create_task(user: stranger, target: mr_assigned, action: Task::MENTIONED) end end @@ -158,13 +184,13 @@ describe TaskService, services: true do mr_unassigned.update_attribute(:assignee, john_doe) service.reassigned_merge_request(mr_unassigned, author) - is_expected_to_create_pending_task(user: john_doe, target: mr_unassigned, action: Task::ASSIGNED) + should_create_task(user: john_doe, target: mr_unassigned, action: Task::ASSIGNED) end it 'does not create a task if unassigned' do mr_assigned.update_attribute(:assignee, nil) - is_expected_to_not_create_task { service.reassigned_merge_request(mr_assigned, author) } + should_not_create_any_task { service.reassigned_merge_request(mr_assigned, author) } end end @@ -186,7 +212,7 @@ describe TaskService, services: true do end end - def is_expected_to_create_pending_task(attributes = {}) + def should_create_task(attributes = {}) attributes.reverse_merge!( project: project, author: author, @@ -196,7 +222,17 @@ describe TaskService, services: true do expect(Task.where(attributes).count).to eq 1 end - def is_expected_to_not_create_task + def should_not_create_task(attributes = {}) + attributes.reverse_merge!( + project: project, + author: author, + state: :pending + ) + + expect(Task.where(attributes).count).to eq 0 + end + + def should_not_create_any_task expect { yield }.not_to change(Task, :count) end end -- 2.18.1 From 1d476b0656b3ec24e264d7a7c30bdca83704b3bd Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 17 Feb 2016 17:45:32 -0200 Subject: [PATCH 22/50] Create a pending task when a user is mentioned on a note --- app/assets/stylesheets/pages/tasks.scss | 24 +++++++++++++++++ app/helpers/tasks_helper.rb | 16 ++++++++++++ app/models/note.rb | 2 ++ app/models/task.rb | 6 +++++ app/services/task_service.rb | 17 +++++++++--- app/views/dashboard/tasks/_common.html.haml | 17 ++++++++++++ app/views/dashboard/tasks/_note.html.haml | 26 +++++++++++++++++++ app/views/dashboard/tasks/_task.html.haml | 21 +++------------ .../20160217174422_add_note_to_tasks.rb | 5 ++++ db/schema.rb | 4 ++- spec/models/note_spec.rb | 2 ++ spec/models/task_spec.rb | 16 ++++++++++++ spec/services/task_service_spec.rb | 11 ++++++++ 13 files changed, 146 insertions(+), 21 deletions(-) create mode 100644 app/views/dashboard/tasks/_common.html.haml create mode 100644 app/views/dashboard/tasks/_note.html.haml create mode 100644 db/migrate/20160217174422_add_note_to_tasks.rb diff --git a/app/assets/stylesheets/pages/tasks.scss b/app/assets/stylesheets/pages/tasks.scss index f0e13f4d786..a3dffeed4ab 100644 --- a/app/assets/stylesheets/pages/tasks.scss +++ b/app/assets/stylesheets/pages/tasks.scss @@ -59,6 +59,15 @@ .task-note { word-wrap: break-word; + .md { + color: #7f8fa4; + font-size: $gl-font-size; + + p { + color: #5c5d5e; + } + } + pre { border: none; background: #f9f9f9; @@ -68,10 +77,25 @@ overflow: hidden; } + .note-image-attach { + margin-top: 4px; + margin-left: 0px; + max-width: 200px; + float: none; + } + p:last-child { margin-bottom: 0; } } + + .task-note-icon { + color: #777; + float: left; + font-size: $gl-font-size; + line-height: 16px; + margin-right: 5px; + } } &:last-child { border:none } diff --git a/app/helpers/tasks_helper.rb b/app/helpers/tasks_helper.rb index cf4eab0ef94..6975c1d1604 100644 --- a/app/helpers/tasks_helper.rb +++ b/app/helpers/tasks_helper.rb @@ -22,4 +22,20 @@ module TasksHelper [task.action_name, target].join(" ") end + + def task_note_link_html(task) + link_to task_note_target_path(task) do + "##{task.target_iid}" + end + end + + def task_note_target_path(task) + polymorphic_path([task.project.namespace.becomes(Namespace), + task.project, task.target], anchor: dom_id(task.note)) + end + + def task_note(text, options = {}) + text = first_line_in_markdown(text, 150, options) + sanitize(text, tags: %w(a img b pre code p span)) + end end diff --git a/app/models/note.rb b/app/models/note.rb index b3809ad81e0..73412024f4e 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -37,6 +37,8 @@ class Note < ActiveRecord::Base belongs_to :author, class_name: "User" belongs_to :updated_by, class_name: "User" + has_many :tasks, dependent: :delete_all + delegate :name, to: :project, prefix: true delegate :name, :email, to: :author, prefix: true diff --git a/app/models/task.rb b/app/models/task.rb index c9881991e38..38c6637e456 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -8,6 +8,7 @@ # target_id :integer not null # target_type :string not null # author_id :integer +# note_id :integer # action :integer # state :string not null # created_at :datetime @@ -19,6 +20,7 @@ class Task < ActiveRecord::Base MENTIONED = 2 belongs_to :author, class_name: "User" + belongs_to :note belongs_to :project belongs_to :target, polymorphic: true, touch: true belongs_to :user @@ -52,6 +54,10 @@ class Task < ActiveRecord::Base target.respond_to? :title end + def note_text + note.try(:note) + end + def target_iid target.respond_to?(:iid) ? target.iid : target_id end diff --git a/app/services/task_service.rb b/app/services/task_service.rb index fa615f4cfcf..d97fa146f5c 100644 --- a/app/services/task_service.rb +++ b/app/services/task_service.rb @@ -77,7 +77,17 @@ class TaskService def new_note(note) # Skip system notes, like status changes and cross-references unless note.system - mark_pending_tasks_as_done(note.noteable, note.author) + project = note.project + target = note.noteable + author = note.author + + mark_pending_tasks_as_done(target, author) + + mentioned_users = build_mentioned_users(project, note, author) + + mentioned_users.each do |user| + create_task(project, target, author, user, Task::MENTIONED, note) + end end end @@ -94,14 +104,15 @@ class TaskService private - def create_task(project, target, author, user, action) + def create_task(project, target, author, user, action, note = nil) attributes = { project: project, user_id: user.id, author_id: author.id, target_id: target.id, target_type: target.class.name, - action: action + action: action, + note: note } Task.create(attributes) diff --git a/app/views/dashboard/tasks/_common.html.haml b/app/views/dashboard/tasks/_common.html.haml new file mode 100644 index 00000000000..b6d0c3c03ac --- /dev/null +++ b/app/views/dashboard/tasks/_common.html.haml @@ -0,0 +1,17 @@ +.task-title + %span.author_name= link_to_author task + %span.task_label{class: task.action_name} + = task_action_name(task) + + %strong= link_to "##{task.target_iid}", [task.project.namespace.becomes(Namespace), task.project, task.target] + + · #{time_ago_with_tooltip(task.created_at)} + +- if task.pending? + .task-actions.pull-right + = link_to 'Done', [:dashboard, task], method: :delete, class: 'btn' + +- if task.body? + .task-body + .task-note + = task.target.title diff --git a/app/views/dashboard/tasks/_note.html.haml b/app/views/dashboard/tasks/_note.html.haml new file mode 100644 index 00000000000..2cfd55afccb --- /dev/null +++ b/app/views/dashboard/tasks/_note.html.haml @@ -0,0 +1,26 @@ +.task-title + %span.author_name + = link_to_author task + %span.task_label{class: task.action_name} + = task_action_name(task) + = task_note_link_html(task) + + · #{time_ago_with_tooltip(task.created_at)} + +- if task.pending? + .task-actions.pull-right + = link_to 'Done', [:dashboard, task], method: :delete, class: 'btn' + +.task-body + .task-note + .md + = task_note(task.note_text, project: task.project) + - note = task.note + - if note.attachment.url + - if note.attachment.image? + = link_to note.attachment.url, target: '_blank' do + = image_tag note.attachment.url, class: 'note-image-attach' + - else + = link_to note.attachment.url, target: "_blank", class: 'note-file-attach' do + %i.fa.fa-paperclip + = note.attachment_identifier diff --git a/app/views/dashboard/tasks/_task.html.haml b/app/views/dashboard/tasks/_task.html.haml index b33f3894fd3..2ca8f0dad63 100644 --- a/app/views/dashboard/tasks/_task.html.haml +++ b/app/views/dashboard/tasks/_task.html.haml @@ -2,20 +2,7 @@ .task-item{class: "#{task.body? ? 'task-block' : 'task-inline' }"} = image_tag avatar_icon(task.author_email, 40), class: "avatar s40", alt:'' - .task-title - %span.author_name= link_to_author task - %span.task_label{class: task.action_name} - = task_action_name(task) - - %strong= link_to "##{task.target_iid}", [task.project.namespace.becomes(Namespace), task.project, task.target] - - · #{time_ago_with_tooltip(task.created_at)} - - - if task.pending? - .task-actions.pull-right - = link_to 'Done', [:dashboard, task], method: :delete, class: 'btn' - - - if task.body? - .task-body - .task-note - = task.target.title + - if task.note.present? + = render 'note', task: task + - else + = render 'common', task: task diff --git a/db/migrate/20160217174422_add_note_to_tasks.rb b/db/migrate/20160217174422_add_note_to_tasks.rb new file mode 100644 index 00000000000..da5cb2e05db --- /dev/null +++ b/db/migrate/20160217174422_add_note_to_tasks.rb @@ -0,0 +1,5 @@ +class AddNoteToTasks < ActiveRecord::Migration + def change + add_reference :tasks, :note, index: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 183227a91ca..2b726d17682 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160217100506) do +ActiveRecord::Schema.define(version: 20160217174422) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -834,9 +834,11 @@ ActiveRecord::Schema.define(version: 20160217100506) do t.string "state", null: false t.datetime "created_at" t.datetime "updated_at" + t.integer "note_id" end add_index "tasks", ["author_id"], name: "index_tasks_on_author_id", using: :btree + add_index "tasks", ["note_id"], name: "index_tasks_on_note_id", using: :btree add_index "tasks", ["project_id"], name: "index_tasks_on_project_id", using: :btree add_index "tasks", ["state"], name: "index_tasks_on_state", using: :btree add_index "tasks", ["target_type", "target_id"], name: "index_tasks_on_target_type_and_target_id", using: :btree diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index e6da3724d33..e146f53c3f7 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -26,6 +26,8 @@ describe Note, models: true do it { is_expected.to belong_to(:project) } it { is_expected.to belong_to(:noteable) } it { is_expected.to belong_to(:author).class_name('User') } + + it { is_expected.to have_many(:tasks).dependent(:delete_all) } end describe 'validation' do diff --git a/spec/models/task_spec.rb b/spec/models/task_spec.rb index 86317626cc3..2f0b51ffc61 100644 --- a/spec/models/task_spec.rb +++ b/spec/models/task_spec.rb @@ -8,6 +8,7 @@ # target_id :integer not null # target_type :string not null # author_id :integer +# note_id :integer # action :integer # state :string not null # created_at :datetime @@ -19,6 +20,7 @@ require 'spec_helper' describe Task, models: true do describe 'relationships' do it { is_expected.to belong_to(:author).class_name("User") } + it { is_expected.to belong_to(:note) } it { is_expected.to belong_to(:project) } it { is_expected.to belong_to(:target).touch(true) } it { is_expected.to belong_to(:user) } @@ -48,6 +50,20 @@ describe Task, models: true do it 'returns false when target does not respond to title' end + describe '#note_text' do + it 'returns nil when note is blank' do + subject.note = nil + + expect(subject.note_text).to be_nil + end + + it 'returns note when note is present' do + subject.note = build(:note, note: 'quick fix') + + expect(subject.note_text).to eq 'quick fix' + end + end + describe '#target_iid' do it 'returns target.iid when target respond to iid' it 'returns target_id when target does not respond to iid' diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb index b35715c8b7a..e373d3a4931 100644 --- a/spec/services/task_service_spec.rb +++ b/spec/services/task_service_spec.rb @@ -128,6 +128,17 @@ describe TaskService, services: true do expect(first_pending_task.reload).to be_pending expect(second_pending_task.reload).to be_pending end + + it 'creates a task for each valid mentioned user' do + note.update_attribute(:note, mentions) + + service.new_note(note) + + should_create_task(user: michael, target: issue, author: john_doe, action: Task::MENTIONED, note: note) + should_create_task(user: author, target: issue, author: john_doe, action: Task::MENTIONED, note: note) + should_not_create_task(user: john_doe, target: issue, author: john_doe, action: Task::MENTIONED, note: note) + should_not_create_task(user: stranger, target: issue, author: john_doe, action: Task::MENTIONED, note: note) + end end end -- 2.18.1 From 4120b7941d75217d013dcc9612e3e5dff76f10d5 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 17 Feb 2016 17:55:45 -0200 Subject: [PATCH 23/50] Does not create a task if new assignee is the current user --- app/services/task_service.rb | 7 +++---- spec/services/task_service_spec.rb | 12 ++++++++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/app/services/task_service.rb b/app/services/task_service.rb index d97fa146f5c..c9b30bcb19c 100644 --- a/app/services/task_service.rb +++ b/app/services/task_service.rb @@ -125,7 +125,6 @@ class TaskService mentioned_users.delete(author) mentioned_users.delete(target.assignee) if target.respond_to?(:assignee) - mentioned_users.uniq end @@ -149,9 +148,9 @@ class TaskService end end - def reassigned_issuable(issuable, user) - if issuable.is_assigned? - create_task(issuable.project, issuable, user, issuable.assignee, Task::ASSIGNED) + def reassigned_issuable(issuable, current_user) + if issuable.is_assigned? && issuable.assignee != current_user + create_task(issuable.project, issuable, current_user, issuable.assignee, Task::ASSIGNED) end end end diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb index e373d3a4931..18ad02dd2df 100644 --- a/spec/services/task_service_spec.rb +++ b/spec/services/task_service_spec.rb @@ -76,6 +76,12 @@ describe TaskService, services: true do should_not_create_any_task { service.reassigned_issue(issue, author) } end + + it 'does not create a task if new assignee is the current user' do + unassigned_issue.update_attribute(:assignee, john_doe) + + should_not_create_any_task { service.reassigned_issue(unassigned_issue, john_doe) } + end end describe '#mark_pending_tasks_as_done' do @@ -203,6 +209,12 @@ describe TaskService, services: true do should_not_create_any_task { service.reassigned_merge_request(mr_assigned, author) } end + + it 'does not create a task if new assignee is the current user' do + mr_assigned.update_attribute(:assignee, john_doe) + + should_not_create_any_task { service.reassigned_merge_request(mr_assigned, john_doe) } + end end describe '#merge_merge_request' do -- 2.18.1 From 3b98adcc75f82f4e5e469da5c164467da02b0f0c Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 17 Feb 2016 20:04:14 -0200 Subject: [PATCH 24/50] Create a pending task when a user is mentioned when edit a issue/mr/note --- app/services/issues/update_service.rb | 2 +- app/services/merge_requests/update_service.rb | 2 +- app/services/task_service.rb | 79 +++++++++-- spec/factories/tasks.rb | 8 +- spec/services/issues/close_service_spec.rb | 4 +- spec/services/issues/update_service_spec.rb | 4 +- .../merge_requests/close_service_spec.rb | 4 +- .../merge_requests/update_service_spec.rb | 4 +- spec/services/notes/update_service_spec.rb | 6 +- spec/services/task_service_spec.rb | 133 +++++++++--------- 10 files changed, 146 insertions(+), 100 deletions(-) diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index 9e39847dc54..042b567b25f 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -6,7 +6,7 @@ module Issues def handle_changes(issue, options = {}) if have_changes?(issue, options) - task_service.mark_pending_tasks_as_done(issue, current_user) + task_service.update_issue(issue, current_user) end if issue.previous_changes.include?('milestone_id') diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 38ae067877a..87949f0a9b8 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -16,7 +16,7 @@ module MergeRequests def handle_changes(merge_request, options = {}) if have_changes?(merge_request, options) - task_service.mark_pending_tasks_as_done(merge_request, current_user) + task_service.update_merge_request(merge_request, current_user) end if merge_request.previous_changes.include?('target_branch') diff --git a/app/services/task_service.rb b/app/services/task_service.rb index c9b30bcb19c..48678763f31 100644 --- a/app/services/task_service.rb +++ b/app/services/task_service.rb @@ -8,12 +8,22 @@ class TaskService # When create an issue we should: # - # * creates a pending task for assignee if issue is assigned + # * create a task for assignee if issue is assigned + # * create a task for each mentioned user on issue # def new_issue(issue, current_user) new_issuable(issue, current_user) end + # When update an issue we should: + # + # * mark all pending tasks related to the issue for the current user as done + # * create a task for each new user mentioned on issue + # + def update_issue(issue, current_user) + update_issuable(issue, current_user) + end + # When close an issue we should: # # * mark all pending tasks related to the target for the current user as done @@ -24,7 +34,7 @@ class TaskService # When we reassign an issue we should: # - # * creates a pending task for new assignee if issue is assigned + # * create a pending task for new assignee if issue is assigned # def reassigned_issue(issue, current_user) reassigned_issuable(issue, current_user) @@ -33,11 +43,21 @@ class TaskService # When create a merge request we should: # # * creates a pending task for assignee if merge request is assigned + # * create a task for each mentioned user on merge request # def new_merge_request(merge_request, current_user) new_issuable(merge_request, current_user) end + # When update a merge request we should: + # + # * mark all pending tasks related to the merge request for the current user as done + # * create a task for each new user mentioned on merge request + # + def update_merge_request(merge_request, current_user) + update_issuable(merge_request, current_user) + end + # When close a merge request we should: # # * mark all pending tasks related to the target for the current user as done @@ -62,17 +82,10 @@ class TaskService mark_pending_tasks_as_done(merge_request, current_user) end - # When we mark a task as done we should: - # - # * mark all pending tasks related to the target for the user as done - # - def mark_pending_tasks_as_done(target, user) - pending_tasks(user, target.project, target).update_all(state: :done) - end - # When create a note we should: # # * mark all pending tasks related to the noteable for the note author as done + # * create a task for each mentioned user on note # def new_note(note) # Skip system notes, like status changes and cross-references @@ -94,11 +107,24 @@ class TaskService # When update a note we should: # # * mark all pending tasks related to the noteable for the current user as done + # * create a task for each new user mentioned on note # def update_note(note, current_user) # Skip system notes, like status changes and cross-references unless note.system - mark_pending_tasks_as_done(note.noteable, current_user) + project = note.project + target = note.noteable + author = current_user + + mark_pending_tasks_as_done(target, author) + + mentioned_users = build_mentioned_users(project, note, author) + + mentioned_users.each do |user| + unless pending_tasks(mentioned_user, project, target, note: note, action: Task::MENTIONED).exists? + create_task(project, target, author, user, Task::MENTIONED, note) + end + end end end @@ -128,8 +154,17 @@ class TaskService mentioned_users.uniq end - def pending_tasks(user, project, target) - user.tasks.pending.where(project: project, target: target) + def mark_pending_tasks_as_done(target, user) + pending_tasks(user, target.project, target).update_all(state: :done) + end + + def pending_tasks(user, project, target, options = {}) + options.reverse_merge({ + project: project, + target: target + }) + + user.tasks.pending.where(options) end def new_issuable(issuable, current_user) @@ -148,8 +183,24 @@ class TaskService end end + def update_issuable(issuable, current_user) + project = issuable.project + target = issuable + author = current_user + + mark_pending_tasks_as_done(target, author) + + mentioned_users = build_mentioned_users(project, target, author) + + mentioned_users.each do |mentioned_user| + unless pending_tasks(mentioned_user, project, target, action: Task::MENTIONED).exists? + create_task(project, target, author, mentioned_user, Task::MENTIONED) + end + end + end + def reassigned_issuable(issuable, current_user) - if issuable.is_assigned? && issuable.assignee != current_user + if issuable.assignee && issuable.assignee != current_user create_task(issuable.project, issuable, current_user, issuable.assignee, Task::ASSIGNED) end end diff --git a/spec/factories/tasks.rb b/spec/factories/tasks.rb index 710b8324f1f..b31db8a7d8b 100644 --- a/spec/factories/tasks.rb +++ b/spec/factories/tasks.rb @@ -20,14 +20,12 @@ FactoryGirl.define do author user - factory :pending_assigned_task, traits: [:assgined, :pending] - - trait :assgined do + trait :assigned do action { Task::ASSIGNED } end - trait :pending do - state { :pending } + trait :mentioned do + action { Task::MENTIONED } end end end diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 7a587a96ddc..fdf0fd819b2 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -5,9 +5,7 @@ describe Issues::CloseService, services: true do let(:user2) { create(:user) } let(:issue) { create(:issue, assignee: user2) } let(:project) { issue.project } - let!(:pending_task) do - create(:pending_assigned_task, user: user, project: project, target: issue, author: user2) - end + let!(:pending_task) { create(:task, :assigned, user: user, project: project, target: issue, author: user2) } before do project.team << [user, :master] diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 1b7d5b45168..5674b9f1e9b 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -81,9 +81,7 @@ describe Issues::UpdateService, services: true do end context 'task queue' do - let!(:pending_task) do - create(:pending_assigned_task, user: user, project: project, target: issue, author: user2) - end + let!(:pending_task) { create(:task, :assigned, user: user, project: project, target: issue, author: user2) } context 'when the title change' do before do diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index 3e72be2dc25..13872a1a2dd 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -5,9 +5,7 @@ describe MergeRequests::CloseService, services: true do let(:user2) { create(:user) } let(:merge_request) { create(:merge_request, assignee: user2) } let(:project) { merge_request.project } - let!(:pending_task) do - create(:pending_assigned_task, user: user, project: project, target: merge_request, author: user2) - end + let!(:pending_task) { create(:task, :assigned, user: user, project: project, target: merge_request, author: user2) } before do project.team << [user, :master] diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 77c661fd293..6d70e8ec16a 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -99,9 +99,7 @@ describe MergeRequests::UpdateService, services: true do end context 'task queue' do - let!(:pending_task) do - create(:pending_assigned_task, user: user, project: project, target: merge_request, author: user2) - end + let!(:pending_task) { create(:task, :assigned, user: user, project: project, target: merge_request, author: user2) } context 'when the title change' do before do diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb index e6670143951..a8b3e0d825d 100644 --- a/spec/services/notes/update_service_spec.rb +++ b/spec/services/notes/update_service_spec.rb @@ -19,7 +19,7 @@ describe Notes::UpdateService, services: true do end context 'task queue' do - let!(:task) { create(:pending_assigned_task, user: user, project: project, target: issue, author: user2) } + let!(:pending_task) { create(:task, :assigned, user: user, project: project, target: issue, author: user2) } context 'when the note change' do before do @@ -27,7 +27,7 @@ describe Notes::UpdateService, services: true do end it 'marks pending tasks as done' do - expect(task.reload).to be_done + expect(pending_task.reload).to be_done end end @@ -37,7 +37,7 @@ describe Notes::UpdateService, services: true do end it 'keep pending tasks' do - expect(task.reload).to be_pending + expect(pending_task.reload).to be_pending end end end diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb index 18ad02dd2df..a5a497c7002 100644 --- a/spec/services/task_service_spec.rb +++ b/spec/services/task_service_spec.rb @@ -16,7 +16,7 @@ describe TaskService, services: true do end describe 'Issues' do - let(:issue) { create(:issue, project: project, assignee: john_doe, author: author) } + let(:issue) { create(:issue, project: project, assignee: john_doe, author: author, description: mentions) } let(:unassigned_issue) { create(:issue, project: project, assignee: nil) } describe '#new_issue' do @@ -35,8 +35,6 @@ describe TaskService, services: true do end it 'creates a task for each valid mentioned user' do - issue.update_attribute(:description, mentions) - service.new_issue(issue, author) should_create_task(user: michael, target: issue, action: Task::MENTIONED) @@ -46,20 +44,39 @@ describe TaskService, services: true do end end - describe '#close_issue' do - let!(:first_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author) + describe '#update_issue' do + it 'marks pending tasks to the issue for the user as done' do + pending_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) + service.update_issue(issue, john_doe) + + expect(pending_task.reload).to be_done + end + + it 'creates a task for each valid mentioned user' do + service.update_issue(issue, author) + + should_create_task(user: michael, target: issue, action: Task::MENTIONED) + should_not_create_task(user: author, target: issue, action: Task::MENTIONED) + should_not_create_task(user: john_doe, target: issue, action: Task::MENTIONED) + should_not_create_task(user: stranger, target: issue, action: Task::MENTIONED) end - let!(:second_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author) + it 'does not create a task if user was already mentioned' do + create(:task, :mentioned, user: michael, project: project, target: issue, author: author) + + should_not_create_any_task { service.update_issue(issue, author) } end + end + describe '#close_issue' do it 'marks related pending tasks to the target for the user as done' do + first_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) + second_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) + service.close_issue(issue, john_doe) - expect(first_pending_task.reload).to be_done - expect(second_pending_task.reload).to be_done + expect(first_task.reload).to be_done + expect(second_task.reload).to be_done end end @@ -84,60 +101,38 @@ describe TaskService, services: true do end end - describe '#mark_pending_tasks_as_done' do - let!(:first_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author) - end - - let!(:second_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author) - end - - it 'marks related pending tasks to the target for the user as done' do - service.mark_pending_tasks_as_done(issue, john_doe) - - expect(first_pending_task.reload).to be_done - expect(second_pending_task.reload).to be_done - end - end - describe '#new_note' do - let!(:first_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author) - end - - let!(:second_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author) - end - - let(:note) { create(:note, project: project, noteable: issue, author: john_doe) } + let!(:first_task) { create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) } + let!(:second_task) { create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) } + let(:note) { create(:note, project: project, noteable: issue, author: john_doe, note: mentions) } let(:award_note) { create(:note, :award, project: project, noteable: issue, author: john_doe, note: 'thumbsup') } let(:system_note) { create(:system_note, project: project, noteable: issue) } it 'mark related pending tasks to the noteable for the note author as done' do + first_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) + second_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) + service.new_note(note) - expect(first_pending_task.reload).to be_done - expect(second_pending_task.reload).to be_done + expect(first_task.reload).to be_done + expect(second_task.reload).to be_done end it 'mark related pending tasks to the noteable for the award note author as done' do service.new_note(award_note) - expect(first_pending_task.reload).to be_done - expect(second_pending_task.reload).to be_done + expect(first_task.reload).to be_done + expect(second_task.reload).to be_done end it 'does not mark related pending tasks it is a system note' do service.new_note(system_note) - expect(first_pending_task.reload).to be_pending - expect(second_pending_task.reload).to be_pending + expect(first_task.reload).to be_pending + expect(second_task.reload).to be_pending end it 'creates a task for each valid mentioned user' do - note.update_attribute(:note, mentions) - service.new_note(note) should_create_task(user: michael, target: issue, author: john_doe, action: Task::MENTIONED, note: note) @@ -149,7 +144,7 @@ describe TaskService, services: true do end describe 'Merge Requests' do - let(:mr_assigned) { create(:merge_request, source_project: project, author: author, assignee: john_doe) } + let(:mr_assigned) { create(:merge_request, source_project: project, author: author, assignee: john_doe, description: mentions) } let(:mr_unassigned) { create(:merge_request, source_project: project, author: author, assignee: nil) } describe '#new_merge_request' do @@ -168,8 +163,6 @@ describe TaskService, services: true do end it 'creates a task for each valid mentioned user' do - mr_assigned.update_attribute(:description, mentions) - service.new_merge_request(mr_assigned, author) should_create_task(user: michael, target: mr_assigned, action: Task::MENTIONED) @@ -179,20 +172,38 @@ describe TaskService, services: true do end end - describe '#close_merge_request' do - let!(:first_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: mr_assigned, author: author) + describe '#update_merge_request' do + it 'marks pending tasks to the merge request for the user as done' do + pending_task = create(:task, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) + service.update_merge_request(mr_assigned, john_doe) + + expect(pending_task.reload).to be_done + end + + it 'creates a task for each valid mentioned user' do + service.update_merge_request(mr_assigned, author) + + should_create_task(user: michael, target: mr_assigned, action: Task::MENTIONED) + should_not_create_task(user: author, target: mr_assigned, action: Task::MENTIONED) + should_not_create_task(user: john_doe, target: mr_assigned, action: Task::MENTIONED) + should_not_create_task(user: stranger, target: mr_assigned, action: Task::MENTIONED) end - let!(:second_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: mr_assigned, author: author) + it 'does not create a task if user was already mentioned' do + create(:task, :mentioned, user: michael, project: project, target: mr_assigned, author: author) + + should_not_create_any_task { service.update_merge_request(mr_assigned, author) } end + end + describe '#close_merge_request' do it 'marks related pending tasks to the target for the user as done' do + first_task = create(:task, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) + second_task = create(:task, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) service.close_merge_request(mr_assigned, john_doe) - expect(first_pending_task.reload).to be_done - expect(second_pending_task.reload).to be_done + expect(first_task.reload).to be_done + expect(second_task.reload).to be_done end end @@ -218,19 +229,13 @@ describe TaskService, services: true do end describe '#merge_merge_request' do - let!(:first_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: mr_assigned, author: author) - end - - let!(:second_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: mr_assigned, author: author) - end - it 'marks related pending tasks to the target for the user as done' do + first_task = create(:task, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) + second_task = create(:task, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) service.merge_merge_request(mr_assigned, john_doe) - expect(first_pending_task.reload).to be_done - expect(second_pending_task.reload).to be_done + expect(first_task.reload).to be_done + expect(second_task.reload).to be_done end end end -- 2.18.1 From c4c4b808f3ff7944e6b4216e910cc40a0fa2ebec Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 17 Feb 2016 20:36:13 -0200 Subject: [PATCH 25/50] Add a link to Task Queue on dashboard sidebar --- app/views/layouts/nav/_dashboard.html.haml | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/app/views/layouts/nav/_dashboard.html.haml b/app/views/layouts/nav/_dashboard.html.haml index 106abd24a56..0b59577ad66 100644 --- a/app/views/layouts/nav/_dashboard.html.haml +++ b/app/views/layouts/nav/_dashboard.html.haml @@ -25,12 +25,18 @@ %span Issues %span.count= number_with_delimiter(current_user.assigned_issues.opened.count) - = nav_link(path: 'dashboard#merge_requests') do - = link_to assigned_mrs_dashboard_path, title: 'Merge Requests', class: 'shortcuts-merge_requests' do - = icon('tasks fw') - %span - Merge Requests - %span.count= number_with_delimiter(current_user.assigned_merge_requests.opened.count) + = nav_link(path: 'dashboard#merge_requests') do + = link_to assigned_mrs_dashboard_path, title: 'Merge Requests', class: 'shortcuts-merge_requests' do + = icon('tasks fw') + %span + Merge Requests + %span.count= number_with_delimiter(current_user.assigned_merge_requests.opened.count) + = nav_link(path: 'dashboard#tasks') do + = link_to dashboard_tasks_path, title: 'Task Queue' do + = icon('bell fw') + %span + Task Queue + %span.count= number_with_delimiter(tasks_pending_count) = nav_link(controller: :snippets) do = link_to dashboard_snippets_path, title: 'Snippets' do = icon('clipboard fw') -- 2.18.1 From 07eb334c2546294acea43e85308bec907b13467c Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 18 Feb 2016 01:20:31 -0200 Subject: [PATCH 26/50] Add filters by project, author, type, and action to task queue page list --- app/controllers/dashboard/tasks_controller.rb | 8 +- app/finders/tasks_finder.rb | 129 ++++++++++++++++++ app/helpers/tasks_helper.rb | 33 +++++ app/views/dashboard/tasks/index.html.haml | 45 +++++- features/dashboard/task_queue.feature | 23 ++++ features/steps/dashboard/task_queue.rb | 116 ++++++++++++---- 6 files changed, 317 insertions(+), 37 deletions(-) create mode 100644 app/finders/tasks_finder.rb diff --git a/app/controllers/dashboard/tasks_controller.rb b/app/controllers/dashboard/tasks_controller.rb index e3245609204..a8884be54e4 100644 --- a/app/controllers/dashboard/tasks_controller.rb +++ b/app/controllers/dashboard/tasks_controller.rb @@ -1,12 +1,6 @@ class Dashboard::TasksController < Dashboard::ApplicationController def index - @tasks = case params[:state] - when 'done' - current_user.tasks.done - else - current_user.tasks.pending - end - + @tasks = TasksFinder.new(current_user, params).execute @tasks = @tasks.page(params[:page]).per(PER_PAGE) end diff --git a/app/finders/tasks_finder.rb b/app/finders/tasks_finder.rb new file mode 100644 index 00000000000..2a32e977c24 --- /dev/null +++ b/app/finders/tasks_finder.rb @@ -0,0 +1,129 @@ +# TasksFinder +# +# Used to filter Tasks by set of params +# +# Arguments: +# current_user - which user use +# params: +# action_id: integer +# author_id: integer +# project_id; integer +# state: 'pending' or 'done' +# type: 'Issue' or 'MergeRequest' +# + +class TasksFinder + NONE = '0' + + attr_accessor :current_user, :params + + def initialize(current_user, params) + @current_user = current_user + @params = params + end + + def execute + items = current_user.tasks + items = by_action_id(items) + items = by_author(items) + items = by_project(items) + items = by_state(items) + items = by_type(items) + + items + end + + private + + def action_id? + action_id.present? && [Task::ASSIGNED, Task::MENTIONED].include?(action_id.to_i) + end + + def action_id + params[:action_id] + 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 + + def project? + params[:project_id].present? + end + + def project + return @project if defined?(@project) + + if project? + @project = Project.find(params[:project_id]) + + unless Ability.abilities.allowed?(current_user, :read_project, @project) + @project = nil + end + else + @project = nil + end + + @project + end + + def type? + type.present? && ['Issue', 'MergeRequest'].include?(type) + end + + def type + params[:type] + end + + def by_action_id(items) + if action_id? + items = items.where(action: action_id) + end + + items + end + + def by_author(items) + if author? + items = items.where(author_id: author.try(:id)) + end + + items + end + + def by_project(items) + if project? + items = items.where(project: project) + end + + items + end + + def by_state(items) + case params[:state] + when 'done' + items.done + else + items.pending + end + end + + def by_type(items) + if type? + items = items.where(target_type: type) + end + + items + end +end diff --git a/app/helpers/tasks_helper.rb b/app/helpers/tasks_helper.rb index 6975c1d1604..59c7d93e65e 100644 --- a/app/helpers/tasks_helper.rb +++ b/app/helpers/tasks_helper.rb @@ -38,4 +38,37 @@ module TasksHelper text = first_line_in_markdown(text, 150, options) sanitize(text, tags: %w(a img b pre code p span)) end + + def task_actions_options + actions = [ + OpenStruct.new(id: '', title: 'Any Action'), + OpenStruct.new(id: Task::ASSIGNED, title: 'Assigned'), + OpenStruct.new(id: Task::MENTIONED, title: 'Mentioned') + ] + + options_from_collection_for_select(actions, 'id', 'title', params[:action_id]) + end + + def task_projects_options + projects = current_user.authorized_projects.sorted_by_activity.non_archived + projects = projects.includes(:namespace) + + projects = projects.map do |project| + OpenStruct.new(id: project.id, title: project.name_with_namespace) + end + + projects.unshift(OpenStruct.new(id: '', title: 'Any Project')) + + options_from_collection_for_select(projects, 'id', 'title', params[:project_id]) + end + + def task_types_options + types = [ + OpenStruct.new(title: 'Any Type', name: ''), + OpenStruct.new(title: 'Issue', name: 'Issue'), + OpenStruct.new(title: 'Merge Request', name: 'MergeRequest') + ] + + options_from_collection_for_select(types, 'name', 'title', params[:type]) + end end diff --git a/app/views/dashboard/tasks/index.html.haml b/app/views/dashboard/tasks/index.html.haml index 2f582009288..16e7b4ae0c5 100644 --- a/app/views/dashboard/tasks/index.html.haml +++ b/app/views/dashboard/tasks/index.html.haml @@ -3,12 +3,37 @@ .top-area %ul.nav-links - %li{class: ("active" if params[:state].blank? || params[:state] == 'pending')} - = link_to dashboard_tasks_path(state: 'pending') do - Tasks (#{tasks_pending_count}) - %li{class: ("active" if params[:state] == 'done')} - = link_to dashboard_tasks_path(state: 'done') do - Done (#{tasks_done_count}) + %li{class: ('active' if params[:state].blank? || params[:state] == 'pending')} + = link_to page_filter_path(state: 'pending') do + %span + Tasks + %span{class: 'badge'} + = tasks_pending_count + %li{class: ('active' if params[:state] == 'done')} + = link_to page_filter_path(state: 'done') do + %span + Done + %span{class: 'badge'} + = tasks_done_count + +.tasks-filters + .gray-content-block.second-block + = form_tag page_filter_path(without: [:assignee_id, :milestone_title, :label_name, :scope, :sort]), method: :get, class: 'filter-form' do + .filter-item.inline + = select_tag('project_id', task_projects_options, + class: 'select2 trigger-submit', include_blank: true, + data: {placeholder: 'Project'}) + .filter-item.inline + = users_select_tag(:author_id, selected: params[:author_id], + placeholder: 'Author', class: 'trigger-submit', any_user: "Any Author", first_user: true, current_user: true) + .filter-item.inline + = select_tag('type', task_types_options, + class: 'select2 trigger-submit', include_blank: true, + data: {placeholder: 'Type'}) + .filter-item.inline.actions-filter + = select_tag('action_id', task_actions_options, + class: 'select2 trigger-submit', include_blank: true, + data: {placeholder: 'Action'}) .tasks - if @tasks.any? @@ -23,3 +48,11 @@ = paginate @tasks, theme: "gitlab" - else .nothing-here-block No tasks to show + +:javascript + new UsersSelect(); + + $('form.filter-form').on('submit', function (event) { + event.preventDefault(); + Turbolinks.visit(this.action + '&' + $(this).serialize()); + }); diff --git a/features/dashboard/task_queue.feature b/features/dashboard/task_queue.feature index 42b4a86e498..8972a148289 100644 --- a/features/dashboard/task_queue.feature +++ b/features/dashboard/task_queue.feature @@ -4,6 +4,9 @@ Feature: Dashboard Task Queue Given I sign in as a user And I own project "Shop" And "John Doe" is a developer of project "Shop" + And "Mary Jane" is a developer of project "Shop" + And "Mary Jane" owns private project "Enterprise" + And I am a developer of project "Enterprise" And I have pending tasks And I visit dashboard task queue page @@ -13,3 +16,23 @@ Feature: Dashboard Task Queue And I mark the pending task as done And I click on the "Done" tab Then I should see all tasks marked as done + + @javascript + Scenario: I filter by project + Given I filter by "Enterprise" + Then I should not see tasks + + @javascript + Scenario: I filter by author + Given I filter by "John Doe" + Then I should not see tasks related to "Mary Jane" in the list + + @javascript + Scenario: I filter by type + Given I filter by "Issue" + Then I should not see tasks related to "Merge Requests" in the list + + @javascript + Scenario: I filter by action + Given I filter by "Mentioned" + Then I should not see tasks related to "Assignments" in the list diff --git a/features/steps/dashboard/task_queue.rb b/features/steps/dashboard/task_queue.rb index 8695dd5cfb1..53920f585dc 100644 --- a/features/steps/dashboard/task_queue.rb +++ b/features/steps/dashboard/task_queue.rb @@ -3,58 +3,126 @@ class Spinach::Features::DashboardTaskQueue < Spinach::FeatureSteps include SharedPaths include SharedProject include SharedUser + include Select2Helper step '"John Doe" is a developer of project "Shop"' do project.team << [john_doe, :developer] end + step 'I am a developer of project "Enterprise"' do + enterprise.team << [current_user, :developer] + end + + step '"Mary Jane" is a developer of project "Shop"' do + project.team << [john_doe, :developer] + end + step 'I have pending tasks' do - create(:task, user: current_user, project: project, author: john_doe, target: assigned_issue, action: Task::ASSIGNED) + create(:task, user: current_user, project: project, author: mary_jane, target: issue, action: Task::MENTIONED) + create(:task, user: current_user, project: project, author: john_doe, target: issue, action: Task::ASSIGNED) + note = create(:note, author: john_doe, noteable: issue, note: "#{current_user.to_reference} Wdyt?") + create(:task, user: current_user, project: project, author: john_doe, target: issue, action: Task::MENTIONED, note: note) + create(:task, user: current_user, project: project, author: john_doe, target: merge_request, action: Task::ASSIGNED) end step 'I should see pending tasks assigned to me' do - expect(page).to have_link 'Tasks (1)' - expect(page).to have_link 'Done (0)' - - page.within('.tasks') do - expect(page).to have_content project.name_with_namespace - expect(page).to have_content "John Doe assigned issue ##{assigned_issue.iid}" - expect(page).to have_content(assigned_issue.title[0..10]) - expect(page).to have_link 'Done' - end + expect(page).to have_content 'Tasks 4' + expect(page).to have_content 'Done 0' + + expect(page).to have_link project.name_with_namespace + should_see_task(1, "John Doe assigned merge request ##{merge_request.iid}", merge_request.title) + should_see_task(2, "John Doe mentioned on issue ##{issue.iid}", "#{current_user.to_reference} Wdyt?") + should_see_task(3, "John Doe assigned issue ##{issue.iid}", issue.title) + should_see_task(4, "Mary Jane mentioned on issue ##{issue.iid}", issue.title) end step 'I mark the pending task as done' do - click_link 'Done' + page.within('.task:nth-child(1)') do + click_link 'Done' + end expect(page).to have_content 'Task was successfully marked as done.' - expect(page).to have_link 'Tasks (0)' - expect(page).to have_link 'Done (1)' - expect(page).to have_content 'No tasks to show' + expect(page).to have_content 'Tasks 3' + expect(page).to have_content 'Done 1' + should_not_see_task "John Doe assigned merge request ##{merge_request.iid}" end step 'I click on the "Done" tab' do - click_link 'Done (1)' + click_link 'Done 1' end step 'I should see all tasks marked as done' do - page.within('.tasks') do - expect(page).to have_content project.name_with_namespace - expect(page).to have_content "John Doe assigned issue ##{assigned_issue.iid}" - expect(page).to have_content(assigned_issue.title[0..10]) - expect(page).not_to have_link 'Done' + expect(page).to have_link project.name_with_namespace + should_see_task(1, "John Doe assigned merge request ##{merge_request.iid}", merge_request.title, false) + end + + step 'I filter by "Enterprise"' do + select2(enterprise.id, from: "#project_id") + end + + step 'I filter by "John Doe"' do + select2(john_doe.id, from: "#author_id") + end + + step 'I filter by "Issue"' do + select2('Issue', from: "#type") + end + + step 'I filter by "Mentioned"' do + select2("#{Task::MENTIONED}", from: '#action_id') + end + + step 'I should not see tasks' do + expect(page).to have_content 'No tasks to show' + end + + step 'I should not see tasks related to "Mary Jane" in the list' do + should_not_see_task "Mary Jane mentioned on issue ##{issue.iid}" + end + + step 'I should not see tasks related to "Merge Requests" in the list' do + should_not_see_task "John Doe assigned merge request ##{merge_request.iid}" + end + + step 'I should not see tasks related to "Assignments" in the list' do + should_not_see_task "John Doe assigned merge request ##{merge_request.iid}" + should_not_see_task "John Doe assigned issue ##{issue.iid}" + end + + def should_see_task(position, title, body, pending = true) + page.within(".task:nth-child(#{position})") do + expect(page).to have_content title + expect(page).to have_content body + + if pending + expect(page).to have_link 'Done' + else + expect(page).to_not have_link 'Done' + end end end - def assigned_issue - @assigned_issue ||= create(:issue, assignee: current_user, project: project) + def should_not_see_task(title) + expect(page).not_to have_content title end def john_doe @john_doe ||= user_exists("John Doe", { username: "john_doe" }) end - def project - @project ||= create(:project, name: "Shop") + def mary_jane + @mary_jane ||= user_exists("Mary Jane", { username: "mary_jane" }) + end + + def enterprise + @enterprise ||= Project.find_by(name: 'Enterprise') + end + + def issue + @issue ||= create(:issue, assignee: current_user, project: project) + end + + def merge_request + @merge_request ||= create(:merge_request, assignee: current_user, source_project: project) end end -- 2.18.1 From b13a73446a5cda8076b7d4639ff7e70bc1cdbca1 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 18 Feb 2016 01:28:36 -0200 Subject: [PATCH 27/50] Fix pending examples from task model spec --- spec/models/task_spec.rb | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/spec/models/task_spec.rb b/spec/models/task_spec.rb index 2f0b51ffc61..2871e02aa4d 100644 --- a/spec/models/task_spec.rb +++ b/spec/models/task_spec.rb @@ -46,8 +46,21 @@ describe Task, models: true do end describe '#body?' do - it 'returns true when target respond to title' - it 'returns false when target does not respond to title' + let(:issue) { build(:issue) } + + before do + subject.target = issue + end + + it 'returns true when target respond to title' do + expect(subject.body?).to eq true + end + + it 'returns false when target does not respond to title' do + allow(issue).to receive(:respond_to?).with(:title).and_return(false) + + expect(subject.body?).to eq false + end end describe '#note_text' do @@ -65,7 +78,20 @@ describe Task, models: true do end describe '#target_iid' do - it 'returns target.iid when target respond to iid' - it 'returns target_id when target does not respond to iid' + let(:issue) { build(:issue, id: 1, iid: 5) } + + before do + subject.target = issue + end + + it 'returns target.iid when target respond to iid' do + expect(subject.target_iid).to eq 5 + end + + it 'returns target_id when target does not respond to iid' do + allow(issue).to receive(:respond_to?).with(:iid).and_return(false) + + expect(subject.target_iid).to eq 1 + end end end -- 2.18.1 From 1bda6fdf6eceac6db8cff13d9e79fded3f41e407 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 18 Feb 2016 01:29:17 -0200 Subject: [PATCH 28/50] Update CHANGELOG --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 8a95909eee9..1225ce42683 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -75,6 +75,7 @@ v 8.5.0 (unreleased) - Emoji comment on diffs are not award emoji - Add label description (Nuttanart Pornprasitsakul) - Show label row when filtering issues or merge requests by label (Nuttanart Pornprasitsakul) + - Add Task Queue v 8.4.4 - Update omniauth-saml gem to 1.4.2 -- 2.18.1 From 139c513da609d2def0d4122789e94fe29b557e67 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 18 Feb 2016 11:37:35 -0200 Subject: [PATCH 29/50] Fix rubocop offenses --- app/helpers/tasks_helper.rb | 2 +- spec/services/merge_requests/close_service_spec.rb | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/helpers/tasks_helper.rb b/app/helpers/tasks_helper.rb index 59c7d93e65e..a99f2b48a26 100644 --- a/app/helpers/tasks_helper.rb +++ b/app/helpers/tasks_helper.rb @@ -31,7 +31,7 @@ module TasksHelper def task_note_target_path(task) polymorphic_path([task.project.namespace.becomes(Namespace), - task.project, task.target], anchor: dom_id(task.note)) + task.project, task.target], anchor: dom_id(task.note)) end def task_note(text, options = {}) diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index 13872a1a2dd..50227af7f2b 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -43,9 +43,9 @@ describe MergeRequests::CloseService, services: true do expect(note.note).to include 'Status changed to closed' end - it 'marks pending tasks as done' do - expect(pending_task.reload).to be_done - end + it 'marks pending tasks as done' do + expect(pending_task.reload).to be_done + end end end end -- 2.18.1 From bc54300ce5329b66d0ba726f814d9e761e782668 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 18 Feb 2016 11:51:53 -0200 Subject: [PATCH 30/50] Fix task factory --- app/models/task.rb | 2 +- spec/factories/tasks.rb | 5 ++++- spec/models/task_spec.rb | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/models/task.rb b/app/models/task.rb index 38c6637e456..cc1f501a7ec 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -9,7 +9,7 @@ # target_type :string not null # author_id :integer # note_id :integer -# action :integer +# action :integer not null # state :string not null # created_at :datetime # updated_at :datetime diff --git a/spec/factories/tasks.rb b/spec/factories/tasks.rb index b31db8a7d8b..4df489fa4c9 100644 --- a/spec/factories/tasks.rb +++ b/spec/factories/tasks.rb @@ -8,7 +8,8 @@ # target_id :integer not null # target_type :string not null # author_id :integer -# action :integer +# note_id :integer +# action :integer not null # state :string not null # created_at :datetime # updated_at :datetime @@ -19,6 +20,8 @@ FactoryGirl.define do project author user + target factory: :issue + action { Task::ASSIGNED } trait :assigned do action { Task::ASSIGNED } diff --git a/spec/models/task_spec.rb b/spec/models/task_spec.rb index 2871e02aa4d..916f618471f 100644 --- a/spec/models/task_spec.rb +++ b/spec/models/task_spec.rb @@ -9,7 +9,7 @@ # target_type :string not null # author_id :integer # note_id :integer -# action :integer +# action :integer not null # state :string not null # created_at :datetime # updated_at :datetime -- 2.18.1 From a74a69db689b7e36c0e275a1094b1ad757bd6e86 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 18 Feb 2016 16:36:30 -0200 Subject: [PATCH 31/50] Use destroy, in case we ever have before_destroy callbacks on Task --- app/models/note.rb | 2 +- spec/models/note_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/note.rb b/app/models/note.rb index 73412024f4e..31606cf8222 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -37,7 +37,7 @@ class Note < ActiveRecord::Base belongs_to :author, class_name: "User" belongs_to :updated_by, class_name: "User" - has_many :tasks, dependent: :delete_all + has_many :tasks, dependent: :destroy delegate :name, to: :project, prefix: true delegate :name, :email, to: :author, prefix: true diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index e146f53c3f7..70c3a999c95 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -27,7 +27,7 @@ describe Note, models: true do it { is_expected.to belong_to(:noteable) } it { is_expected.to belong_to(:author).class_name('User') } - it { is_expected.to have_many(:tasks).dependent(:delete_all) } + it { is_expected.to have_many(:tasks).dependent(:destroy) } end describe 'validation' do -- 2.18.1 From 8673a70f500ae5b0e93336f9213947f0aba67033 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 18 Feb 2016 16:37:41 -0200 Subject: [PATCH 32/50] Improve formatted message for tasks when action is a mention --- app/models/task.rb | 2 +- features/steps/dashboard/task_queue.rb | 6 +++--- spec/models/task_spec.rb | 8 +++++++- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/app/models/task.rb b/app/models/task.rb index cc1f501a7ec..9b11698221d 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -46,7 +46,7 @@ class Task < ActiveRecord::Base def action_name case action when ASSIGNED then 'assigned' - when MENTIONED then 'mentioned on' + when MENTIONED then 'mentioned you on' end end diff --git a/features/steps/dashboard/task_queue.rb b/features/steps/dashboard/task_queue.rb index 53920f585dc..6ff0aff6c21 100644 --- a/features/steps/dashboard/task_queue.rb +++ b/features/steps/dashboard/task_queue.rb @@ -31,9 +31,9 @@ class Spinach::Features::DashboardTaskQueue < Spinach::FeatureSteps expect(page).to have_link project.name_with_namespace should_see_task(1, "John Doe assigned merge request ##{merge_request.iid}", merge_request.title) - should_see_task(2, "John Doe mentioned on issue ##{issue.iid}", "#{current_user.to_reference} Wdyt?") + should_see_task(2, "John Doe mentioned you on issue ##{issue.iid}", "#{current_user.to_reference} Wdyt?") should_see_task(3, "John Doe assigned issue ##{issue.iid}", issue.title) - should_see_task(4, "Mary Jane mentioned on issue ##{issue.iid}", issue.title) + should_see_task(4, "Mary Jane mentioned you on issue ##{issue.iid}", issue.title) end step 'I mark the pending task as done' do @@ -77,7 +77,7 @@ class Spinach::Features::DashboardTaskQueue < Spinach::FeatureSteps end step 'I should not see tasks related to "Mary Jane" in the list' do - should_not_see_task "Mary Jane mentioned on issue ##{issue.iid}" + should_not_see_task "Mary Jane mentioned you on issue ##{issue.iid}" end step 'I should not see tasks related to "Merge Requests" in the list' do diff --git a/spec/models/task_spec.rb b/spec/models/task_spec.rb index 916f618471f..2d00c7dbc8c 100644 --- a/spec/models/task_spec.rb +++ b/spec/models/task_spec.rb @@ -38,11 +38,17 @@ describe Task, models: true do end describe '#action_name' do - it 'returns assigned when action is assigned' do + it 'returns proper message when action is an assigment' do subject.action = Task::ASSIGNED expect(subject.action_name).to eq 'assigned' end + + it 'returns proper message when action is a mention' do + subject.action = Task::MENTIONED + + expect(subject.action_name).to eq 'mentioned you on' + end end describe '#body?' do -- 2.18.1 From 72009896758dfcf73f94146b67e7758190ce7039 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 18 Feb 2016 17:16:39 -0200 Subject: [PATCH 33/50] Refactoring task queue partials --- app/helpers/tasks_helper.rb | 12 ++++++---- app/models/task.rb | 12 +++++----- app/views/dashboard/tasks/_common.html.haml | 17 -------------- app/views/dashboard/tasks/_note.html.haml | 26 --------------------- app/views/dashboard/tasks/_task.html.haml | 25 +++++++++++++++----- spec/models/task_spec.rb | 24 ++++--------------- 6 files changed, 37 insertions(+), 79 deletions(-) delete mode 100644 app/views/dashboard/tasks/_common.html.haml delete mode 100644 app/views/dashboard/tasks/_note.html.haml diff --git a/app/helpers/tasks_helper.rb b/app/helpers/tasks_helper.rb index a99f2b48a26..d8e0780e4c1 100644 --- a/app/helpers/tasks_helper.rb +++ b/app/helpers/tasks_helper.rb @@ -23,18 +23,20 @@ module TasksHelper [task.action_name, target].join(" ") end - def task_note_link_html(task) - link_to task_note_target_path(task) do + def task_target_link_html(task) + link_to task_target_path(task) do "##{task.target_iid}" end end - def task_note_target_path(task) + def task_target_path(task) + anchor = dom_id(task.note) if task.note.present? + polymorphic_path([task.project.namespace.becomes(Namespace), - task.project, task.target], anchor: dom_id(task.note)) + task.project, task.target], anchor: anchor) end - def task_note(text, options = {}) + def task_body(text, options = {}) text = first_line_in_markdown(text, 150, options) sanitize(text, tags: %w(a img b pre code p span)) end diff --git a/app/models/task.rb b/app/models/task.rb index 9b11698221d..0872743097c 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -50,12 +50,12 @@ class Task < ActiveRecord::Base end end - def body? - target.respond_to? :title - end - - def note_text - note.try(:note) + def body + if note.present? + note.note + else + target.title + end end def target_iid diff --git a/app/views/dashboard/tasks/_common.html.haml b/app/views/dashboard/tasks/_common.html.haml deleted file mode 100644 index b6d0c3c03ac..00000000000 --- a/app/views/dashboard/tasks/_common.html.haml +++ /dev/null @@ -1,17 +0,0 @@ -.task-title - %span.author_name= link_to_author task - %span.task_label{class: task.action_name} - = task_action_name(task) - - %strong= link_to "##{task.target_iid}", [task.project.namespace.becomes(Namespace), task.project, task.target] - - · #{time_ago_with_tooltip(task.created_at)} - -- if task.pending? - .task-actions.pull-right - = link_to 'Done', [:dashboard, task], method: :delete, class: 'btn' - -- if task.body? - .task-body - .task-note - = task.target.title diff --git a/app/views/dashboard/tasks/_note.html.haml b/app/views/dashboard/tasks/_note.html.haml deleted file mode 100644 index 2cfd55afccb..00000000000 --- a/app/views/dashboard/tasks/_note.html.haml +++ /dev/null @@ -1,26 +0,0 @@ -.task-title - %span.author_name - = link_to_author task - %span.task_label{class: task.action_name} - = task_action_name(task) - = task_note_link_html(task) - - · #{time_ago_with_tooltip(task.created_at)} - -- if task.pending? - .task-actions.pull-right - = link_to 'Done', [:dashboard, task], method: :delete, class: 'btn' - -.task-body - .task-note - .md - = task_note(task.note_text, project: task.project) - - note = task.note - - if note.attachment.url - - if note.attachment.image? - = link_to note.attachment.url, target: '_blank' do - = image_tag note.attachment.url, class: 'note-image-attach' - - else - = link_to note.attachment.url, target: "_blank", class: 'note-file-attach' do - %i.fa.fa-paperclip - = note.attachment_identifier diff --git a/app/views/dashboard/tasks/_task.html.haml b/app/views/dashboard/tasks/_task.html.haml index 2ca8f0dad63..d08b021f53b 100644 --- a/app/views/dashboard/tasks/_task.html.haml +++ b/app/views/dashboard/tasks/_task.html.haml @@ -1,8 +1,21 @@ %li{class: "task task-#{task.done? ? 'done' : 'pending'}", id: dom_id(task) } - .task-item{class: "#{task.body? ? 'task-block' : 'task-inline' }"} - = image_tag avatar_icon(task.author_email, 40), class: "avatar s40", alt:'' + .task-item{class: 'task-block'} + = image_tag avatar_icon(task.author_email, 40), class: 'avatar s40', alt:'' - - if task.note.present? - = render 'note', task: task - - else - = render 'common', task: task + .task-title + %span.author_name + = link_to_author task + %span.task_label + = task_action_name(task) + = task_target_link_html(task) + + · #{time_ago_with_tooltip(task.created_at)} + + - if task.pending? + .task-actions.pull-right + = link_to 'Done', [:dashboard, task], method: :delete, class: 'btn' + + .task-body + .task-note + .md + = task_body(task.body, project: task.project) diff --git a/spec/models/task_spec.rb b/spec/models/task_spec.rb index 2d00c7dbc8c..2bbd47c5e8a 100644 --- a/spec/models/task_spec.rb +++ b/spec/models/task_spec.rb @@ -51,35 +51,21 @@ describe Task, models: true do end end - describe '#body?' do - let(:issue) { build(:issue) } - + describe '#body' do before do - subject.target = issue - end - - it 'returns true when target respond to title' do - expect(subject.body?).to eq true + subject.target = build(:issue, title: 'Bugfix') end - it 'returns false when target does not respond to title' do - allow(issue).to receive(:respond_to?).with(:title).and_return(false) - - expect(subject.body?).to eq false - end - end - - describe '#note_text' do - it 'returns nil when note is blank' do + it 'returns target title when note is blank' do subject.note = nil - expect(subject.note_text).to be_nil + expect(subject.body).to eq 'Bugfix' end it 'returns note when note is present' do subject.note = build(:note, note: 'quick fix') - expect(subject.note_text).to eq 'quick fix' + expect(subject.body).to eq 'quick fix' end end -- 2.18.1 From 424cb9ccaef35717bd934dd48f8bd69515210a50 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 18 Feb 2016 17:26:52 -0200 Subject: [PATCH 34/50] :lipstick: Cosmetic changes --- app/services/task_service.rb | 42 ++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/app/services/task_service.rb b/app/services/task_service.rb index 48678763f31..cdd025eadc5 100644 --- a/app/services/task_service.rb +++ b/app/services/task_service.rb @@ -89,18 +89,18 @@ class TaskService # def new_note(note) # Skip system notes, like status changes and cross-references - unless note.system - project = note.project - target = note.noteable - author = note.author + return if note.system - mark_pending_tasks_as_done(target, author) + project = note.project + target = note.noteable + author = note.author - mentioned_users = build_mentioned_users(project, note, author) + mark_pending_tasks_as_done(target, author) - mentioned_users.each do |user| - create_task(project, target, author, user, Task::MENTIONED, note) - end + mentioned_users = build_mentioned_users(project, note, author) + + mentioned_users.each do |user| + create_task(project, target, author, user, Task::MENTIONED, note) end end @@ -111,19 +111,19 @@ class TaskService # def update_note(note, current_user) # Skip system notes, like status changes and cross-references - unless note.system - project = note.project - target = note.noteable - author = current_user + return if note.system - mark_pending_tasks_as_done(target, author) + project = note.project + target = note.noteable + author = current_user + + mark_pending_tasks_as_done(target, author) - mentioned_users = build_mentioned_users(project, note, author) + mentioned_users = build_mentioned_users(project, note, author) - mentioned_users.each do |user| - unless pending_tasks(mentioned_user, project, target, note: note, action: Task::MENTIONED).exists? - create_task(project, target, author, user, Task::MENTIONED, note) - end + mentioned_users.each do |user| + unless pending_tasks(mentioned_user, project, target, note: note, action: Task::MENTIONED).exists? + create_task(project, target, author, user, Task::MENTIONED, note) end end end @@ -159,10 +159,10 @@ class TaskService end def pending_tasks(user, project, target, options = {}) - options.reverse_merge({ + options.reverse_merge( project: project, target: target - }) + ) user.tasks.pending.where(options) end -- 2.18.1 From 44656136475d8842628d0a1112aecc9ec412a16f Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 18 Feb 2016 18:09:21 -0200 Subject: [PATCH 35/50] Explicit mention of the assignee make a task Since potentially the previous assign-task has already been handled. --- app/services/task_service.rb | 11 +++++------ spec/services/task_service_spec.rb | 8 ++++---- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/app/services/task_service.rb b/app/services/task_service.rb index cdd025eadc5..bfd724a0f51 100644 --- a/app/services/task_service.rb +++ b/app/services/task_service.rb @@ -150,7 +150,6 @@ class TaskService end mentioned_users.delete(author) - mentioned_users.delete(target.assignee) if target.respond_to?(:assignee) mentioned_users.uniq end @@ -177,6 +176,7 @@ class TaskService end mentioned_users = build_mentioned_users(project, target, author) + mentioned_users.delete(issuable.assignee) mentioned_users.each do |mentioned_user| create_task(project, target, author, mentioned_user, Task::MENTIONED) @@ -185,16 +185,15 @@ class TaskService def update_issuable(issuable, current_user) project = issuable.project - target = issuable author = current_user - mark_pending_tasks_as_done(target, author) + mark_pending_tasks_as_done(issuable, author) - mentioned_users = build_mentioned_users(project, target, author) + mentioned_users = build_mentioned_users(project, issuable, author) mentioned_users.each do |mentioned_user| - unless pending_tasks(mentioned_user, project, target, action: Task::MENTIONED).exists? - create_task(project, target, author, mentioned_user, Task::MENTIONED) + unless pending_tasks(mentioned_user, project, issuable, action: Task::MENTIONED).exists? + create_task(project, issuable, author, mentioned_user, Task::MENTIONED) end end end diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb index a5a497c7002..75498514093 100644 --- a/spec/services/task_service_spec.rb +++ b/spec/services/task_service_spec.rb @@ -56,15 +56,15 @@ describe TaskService, services: true do service.update_issue(issue, author) should_create_task(user: michael, target: issue, action: Task::MENTIONED) + should_create_task(user: john_doe, target: issue, action: Task::MENTIONED) should_not_create_task(user: author, target: issue, action: Task::MENTIONED) - should_not_create_task(user: john_doe, target: issue, action: Task::MENTIONED) should_not_create_task(user: stranger, target: issue, action: Task::MENTIONED) end it 'does not create a task if user was already mentioned' do create(:task, :mentioned, user: michael, project: project, target: issue, author: author) - should_not_create_any_task { service.update_issue(issue, author) } + expect { service.update_issue(issue, author) }.not_to change(michael.tasks, :count) end end @@ -184,15 +184,15 @@ describe TaskService, services: true do service.update_merge_request(mr_assigned, author) should_create_task(user: michael, target: mr_assigned, action: Task::MENTIONED) + should_create_task(user: john_doe, target: mr_assigned, action: Task::MENTIONED) should_not_create_task(user: author, target: mr_assigned, action: Task::MENTIONED) - should_not_create_task(user: john_doe, target: mr_assigned, action: Task::MENTIONED) should_not_create_task(user: stranger, target: mr_assigned, action: Task::MENTIONED) end it 'does not create a task if user was already mentioned' do create(:task, :mentioned, user: michael, project: project, target: mr_assigned, author: author) - should_not_create_any_task { service.update_merge_request(mr_assigned, author) } + expect { service.update_merge_request(mr_assigned, author) }.not_to change(michael.tasks, :count) end end -- 2.18.1 From fc3f8a8ff75a09aae62b2a56c7f78fd9d21d2af3 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 18 Feb 2016 19:12:52 -0200 Subject: [PATCH 36/50] Ensure that we only have one task per issue/mr --- app/services/issues/update_service.rb | 5 + app/services/merge_requests/update_service.rb | 5 + app/services/task_service.rb | 148 +++++++----------- spec/services/task_service_spec.rb | 34 ++-- 4 files changed, 82 insertions(+), 110 deletions(-) diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index 042b567b25f..d2620750305 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -6,6 +6,11 @@ module Issues def handle_changes(issue, options = {}) if have_changes?(issue, options) + task_service.mark_pending_tasks_as_done(issue, current_user) + end + + if issue.previous_changes.include?('title') || + issue.previous_changes.include?('description') task_service.update_issue(issue, current_user) end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 87949f0a9b8..a89daf9821e 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -16,6 +16,11 @@ module MergeRequests def handle_changes(merge_request, options = {}) if have_changes?(merge_request, options) + task_service.mark_pending_tasks_as_done(merge_request, current_user) + end + + if merge_request.previous_changes.include?('title') || + merge_request.previous_changes.include?('description') task_service.update_merge_request(merge_request, current_user) end diff --git a/app/services/task_service.rb b/app/services/task_service.rb index bfd724a0f51..c4479fe6382 100644 --- a/app/services/task_service.rb +++ b/app/services/task_service.rb @@ -18,10 +18,9 @@ class TaskService # When update an issue we should: # # * mark all pending tasks related to the issue for the current user as done - # * create a task for each new user mentioned on issue # def update_issue(issue, current_user) - update_issuable(issue, current_user) + create_mention_tasks(issue.project, issue, current_user) end # When close an issue we should: @@ -37,7 +36,7 @@ class TaskService # * create a pending task for new assignee if issue is assigned # def reassigned_issue(issue, current_user) - reassigned_issuable(issue, current_user) + create_assignment_task(issue, current_user) end # When create a merge request we should: @@ -51,11 +50,10 @@ class TaskService # When update a merge request we should: # - # * mark all pending tasks related to the merge request for the current user as done - # * create a task for each new user mentioned on merge request + # * create a task for each mentioned user on merge request # def update_merge_request(merge_request, current_user) - update_issuable(merge_request, current_user) + create_mention_tasks(merge_request.project, merge_request, current_user) end # When close a merge request we should: @@ -71,7 +69,7 @@ class TaskService # * creates a pending task for new assignee if merge request is assigned # def reassigned_merge_request(merge_request, current_user) - reassigned_issuable(merge_request, current_user) + create_assignment_task(merge_request, current_user) end # When merge a merge request we should: @@ -87,21 +85,8 @@ class TaskService # * mark all pending tasks related to the noteable for the note author as done # * create a task for each mentioned user on note # - def new_note(note) - # Skip system notes, like status changes and cross-references - return if note.system - - project = note.project - target = note.noteable - author = note.author - - mark_pending_tasks_as_done(target, author) - - mentioned_users = build_mentioned_users(project, note, author) - - mentioned_users.each do |user| - create_task(project, target, author, user, Task::MENTIONED, note) - end + def new_note(note, current_user) + handle_note(note, current_user) end # When update a note we should: @@ -110,41 +95,63 @@ class TaskService # * create a task for each new user mentioned on note # def update_note(note, current_user) + handle_note(note, current_user) + end + + # When marking pending tasks as done we should: + # + # * mark all pending tasks related to the target for the current user as done + # + def mark_pending_tasks_as_done(target, user) + pending_tasks(user, target.project, target).update_all(state: :done) + end + + private + + def create_tasks(project, target, author, users, action, note = nil) + Array(users).each do |user| + next if pending_tasks(user, project, target).exists? + + Task.create( + project: project, + user_id: user.id, + author_id: author.id, + target_id: target.id, + target_type: target.class.name, + action: action, + note: note + ) + end + end + + def new_issuable(issuable, author) + create_assignment_task(issuable, author) + create_mention_tasks(issuable.project, issuable, author) + end + + def handle_note(note, author) # Skip system notes, like status changes and cross-references return if note.system project = note.project target = note.noteable - author = current_user mark_pending_tasks_as_done(target, author) + create_mention_tasks(project, target, author, note) + end - mentioned_users = build_mentioned_users(project, note, author) - - mentioned_users.each do |user| - unless pending_tasks(mentioned_user, project, target, note: note, action: Task::MENTIONED).exists? - create_task(project, target, author, user, Task::MENTIONED, note) - end + def create_assignment_task(issuable, author) + if issuable.assignee && issuable.assignee != author + create_tasks(issuable.project, issuable, author, issuable.assignee, Task::ASSIGNED) end end - private - - def create_task(project, target, author, user, action, note = nil) - attributes = { - project: project, - user_id: user.id, - author_id: author.id, - target_id: target.id, - target_type: target.class.name, - action: action, - note: note - } - - Task.create(attributes) + def create_mention_tasks(project, issuable, author, note = nil) + mentioned_users = filter_mentioned_users(project, note || issuable, author) + create_tasks(project, issuable, author, mentioned_users, Task::MENTIONED, note) end - def build_mentioned_users(project, target, author) + def filter_mentioned_users(project, target, author) mentioned_users = target.mentioned_users.select do |user| user.can?(:read_project, project) end @@ -153,54 +160,11 @@ class TaskService mentioned_users.uniq end - def mark_pending_tasks_as_done(target, user) - pending_tasks(user, target.project, target).update_all(state: :done) - end - - def pending_tasks(user, project, target, options = {}) - options.reverse_merge( - project: project, - target: target + def pending_tasks(user, project, target) + user.tasks.pending.where( + project_id: project.id, + target_id: target.id, + target_type: target.class.name ) - - user.tasks.pending.where(options) - end - - def new_issuable(issuable, current_user) - project = issuable.project - target = issuable - author = issuable.author - - if target.is_assigned? && target.assignee != current_user - create_task(project, target, author, target.assignee, Task::ASSIGNED) - end - - mentioned_users = build_mentioned_users(project, target, author) - mentioned_users.delete(issuable.assignee) - - mentioned_users.each do |mentioned_user| - create_task(project, target, author, mentioned_user, Task::MENTIONED) - end - end - - def update_issuable(issuable, current_user) - project = issuable.project - author = current_user - - mark_pending_tasks_as_done(issuable, author) - - mentioned_users = build_mentioned_users(project, issuable, author) - - mentioned_users.each do |mentioned_user| - unless pending_tasks(mentioned_user, project, issuable, action: Task::MENTIONED).exists? - create_task(project, issuable, author, mentioned_user, Task::MENTIONED) - end - end - end - - def reassigned_issuable(issuable, current_user) - if issuable.assignee && issuable.assignee != current_user - create_task(issuable.project, issuable, current_user, issuable.assignee, Task::ASSIGNED) - end end end diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb index 75498514093..43022ca1604 100644 --- a/spec/services/task_service_spec.rb +++ b/spec/services/task_service_spec.rb @@ -45,13 +45,6 @@ describe TaskService, services: true do end describe '#update_issue' do - it 'marks pending tasks to the issue for the user as done' do - pending_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) - service.update_issue(issue, john_doe) - - expect(pending_task.reload).to be_done - end - it 'creates a task for each valid mentioned user' do service.update_issue(issue, author) @@ -101,6 +94,18 @@ describe TaskService, services: true do end end + describe '#mark_pending_tasks_as_done' do + it 'marks related pending tasks to the target for the user as done' do + first_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) + second_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) + + service.mark_pending_tasks_as_done(issue, john_doe) + + expect(first_task.reload).to be_done + expect(second_task.reload).to be_done + end + end + describe '#new_note' do let!(:first_task) { create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) } let!(:second_task) { create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) } @@ -112,28 +117,28 @@ describe TaskService, services: true do first_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) second_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) - service.new_note(note) + service.new_note(note, john_doe) expect(first_task.reload).to be_done expect(second_task.reload).to be_done end it 'mark related pending tasks to the noteable for the award note author as done' do - service.new_note(award_note) + service.new_note(award_note, john_doe) expect(first_task.reload).to be_done expect(second_task.reload).to be_done end it 'does not mark related pending tasks it is a system note' do - service.new_note(system_note) + service.new_note(system_note, john_doe) expect(first_task.reload).to be_pending expect(second_task.reload).to be_pending end it 'creates a task for each valid mentioned user' do - service.new_note(note) + service.new_note(note, john_doe) should_create_task(user: michael, target: issue, author: john_doe, action: Task::MENTIONED, note: note) should_create_task(user: author, target: issue, author: john_doe, action: Task::MENTIONED, note: note) @@ -173,13 +178,6 @@ describe TaskService, services: true do end describe '#update_merge_request' do - it 'marks pending tasks to the merge request for the user as done' do - pending_task = create(:task, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) - service.update_merge_request(mr_assigned, john_doe) - - expect(pending_task.reload).to be_done - end - it 'creates a task for each valid mentioned user' do service.update_merge_request(mr_assigned, author) -- 2.18.1 From a57bf9bd39816cebec5da3da35a1aabbf7462069 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 18 Feb 2016 20:25:46 -0200 Subject: [PATCH 37/50] Rename menu item and page 'Task queue' to 'Tasks' --- app/views/dashboard/tasks/index.html.haml | 4 ++-- app/views/layouts/header/_default.html.haml | 2 +- app/views/layouts/nav/_dashboard.html.haml | 12 ++++++------ .../dashboard/{task_queue.feature => tasks.feature} | 2 +- features/steps/dashboard/{task_queue.rb => tasks.rb} | 2 +- 5 files changed, 11 insertions(+), 11 deletions(-) rename features/dashboard/{task_queue.feature => tasks.feature} (97%) rename features/steps/dashboard/{task_queue.rb => tasks.rb} (98%) diff --git a/app/views/dashboard/tasks/index.html.haml b/app/views/dashboard/tasks/index.html.haml index 16e7b4ae0c5..949006be886 100644 --- a/app/views/dashboard/tasks/index.html.haml +++ b/app/views/dashboard/tasks/index.html.haml @@ -1,5 +1,5 @@ -- page_title "Task Queue" -- header_title "Task Queue", dashboard_tasks_path +- page_title "Tasks" +- header_title "Tasks", dashboard_tasks_path .top-area %ul.nav-links diff --git a/app/views/layouts/header/_default.html.haml b/app/views/layouts/header/_default.html.haml index d3aec9ae202..3d55c4bba1b 100644 --- a/app/views/layouts/header/_default.html.haml +++ b/app/views/layouts/header/_default.html.haml @@ -22,7 +22,7 @@ = link_to admin_root_path, title: 'Admin Area', data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do = icon('wrench fw') %li - = link_to dashboard_tasks_path, title: 'Task Queue', data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do + = link_to dashboard_tasks_path, title: 'Tasks', data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do %span.badge.tasks-pending-count = tasks_pending_count - if current_user.can_create_project? diff --git a/app/views/layouts/nav/_dashboard.html.haml b/app/views/layouts/nav/_dashboard.html.haml index 0b59577ad66..586eff002df 100644 --- a/app/views/layouts/nav/_dashboard.html.haml +++ b/app/views/layouts/nav/_dashboard.html.haml @@ -4,6 +4,12 @@ = icon('home fw') %span Projects + = nav_link(controller: :tasks) do + = link_to dashboard_tasks_path, title: 'Tasks' do + = icon('bell fw') + %span + Tasks + %span.count= number_with_delimiter(tasks_pending_count) = nav_link(path: 'dashboard#activity') do = link_to activity_dashboard_path, class: 'shortcuts-activity', title: 'Activity' do = icon('dashboard fw') @@ -31,12 +37,6 @@ %span Merge Requests %span.count= number_with_delimiter(current_user.assigned_merge_requests.opened.count) - = nav_link(path: 'dashboard#tasks') do - = link_to dashboard_tasks_path, title: 'Task Queue' do - = icon('bell fw') - %span - Task Queue - %span.count= number_with_delimiter(tasks_pending_count) = nav_link(controller: :snippets) do = link_to dashboard_snippets_path, title: 'Snippets' do = icon('clipboard fw') diff --git a/features/dashboard/task_queue.feature b/features/dashboard/tasks.feature similarity index 97% rename from features/dashboard/task_queue.feature rename to features/dashboard/tasks.feature index 8972a148289..9d4f386f7dd 100644 --- a/features/dashboard/task_queue.feature +++ b/features/dashboard/tasks.feature @@ -1,5 +1,5 @@ @dashboard -Feature: Dashboard Task Queue +Feature: Dashboard Tasks Background: Given I sign in as a user And I own project "Shop" diff --git a/features/steps/dashboard/task_queue.rb b/features/steps/dashboard/tasks.rb similarity index 98% rename from features/steps/dashboard/task_queue.rb rename to features/steps/dashboard/tasks.rb index 6ff0aff6c21..556aa41bad9 100644 --- a/features/steps/dashboard/task_queue.rb +++ b/features/steps/dashboard/tasks.rb @@ -1,4 +1,4 @@ -class Spinach::Features::DashboardTaskQueue < Spinach::FeatureSteps +class Spinach::Features::DashboardTasks < Spinach::FeatureSteps include SharedAuthentication include SharedPaths include SharedProject -- 2.18.1 From ce78813c2abb8e65739a0a95d8044e3dca1bfbe6 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 18 Feb 2016 21:53:22 -0200 Subject: [PATCH 38/50] Create or mark task pending as soon the action happens --- app/controllers/projects/merge_requests_controller.rb | 2 ++ app/services/merge_requests/post_merge_service.rb | 1 - app/services/notes/create_service.rb | 1 + app/services/notes/post_process_service.rb | 2 -- spec/services/notes/post_process_service_spec.rb | 2 +- 5 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 86b8e7bdf2e..be58156a9dc 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -181,6 +181,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController return end + TaskService.new.merge_merge_request(merge_request, current_user) + @merge_request.update(merge_error: nil) if params[:merge_when_build_succeeds].present? && @merge_request.ci_commit && @merge_request.ci_commit.active? diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb index 63c2f1f4249..8f25c5e2496 100644 --- a/app/services/merge_requests/post_merge_service.rb +++ b/app/services/merge_requests/post_merge_service.rb @@ -11,7 +11,6 @@ module MergeRequests create_merge_event(merge_request, current_user) create_note(merge_request) notification_service.merge_mr(merge_request, current_user) - task_service.merge_merge_request(merge_request, current_user) execute_hooks(merge_request, 'merge') end diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index 8d9661167b5..dbbf9e63164 100644 --- a/app/services/notes/create_service.rb +++ b/app/services/notes/create_service.rb @@ -8,6 +8,7 @@ module Notes if note.save # Finish the harder work in the background NewNoteWorker.perform_in(2.seconds, note.id, params) + TaskService.new.new_note(note, current_user) end note diff --git a/app/services/notes/post_process_service.rb b/app/services/notes/post_process_service.rb index 465b246cabc..e818f58d13c 100644 --- a/app/services/notes/post_process_service.rb +++ b/app/services/notes/post_process_service.rb @@ -13,8 +13,6 @@ module Notes @note.create_cross_references! execute_note_hooks end - - TaskService.new.new_note(note) end def hook_data diff --git a/spec/services/notes/post_process_service_spec.rb b/spec/services/notes/post_process_service_spec.rb index 9489b8c6336..d4c50f824c1 100644 --- a/spec/services/notes/post_process_service_spec.rb +++ b/spec/services/notes/post_process_service_spec.rb @@ -20,7 +20,7 @@ describe Notes::PostProcessService, services: true do it do expect(project).to receive(:execute_hooks) expect(project).to receive(:execute_services) - expect_any_instance_of(TaskService).to receive(:new_note).with(@note) + Notes::PostProcessService.new(@note).execute end end -- 2.18.1 From d80678c0da431517498ddf54d3b439498177e696 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Fri, 19 Feb 2016 16:37:29 -0200 Subject: [PATCH 39/50] Improve tasks page list UI --- app/views/dashboard/tasks/index.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/dashboard/tasks/index.html.haml b/app/views/dashboard/tasks/index.html.haml index 949006be886..4b6e3d83e62 100644 --- a/app/views/dashboard/tasks/index.html.haml +++ b/app/views/dashboard/tasks/index.html.haml @@ -35,10 +35,10 @@ class: 'select2 trigger-submit', include_blank: true, data: {placeholder: 'Action'}) -.tasks +.prepend-top-default - if @tasks.any? - @tasks.group_by(&:project).each do |group| - .panel.panel-default + .panel.panel-default.panel-small - project = group[0] .panel-heading = link_to project.name_with_namespace, namespace_project_path(project.namespace, project) -- 2.18.1 From a7dee0ab018d8ce660d7ec778b68291ee3b3689b Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Sat, 20 Feb 2016 10:33:46 -0200 Subject: [PATCH 40/50] Refactoring TasksHelper --- app/helpers/tasks_helper.rb | 9 +-------- app/views/dashboard/tasks/_task.html.haml | 2 +- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/app/helpers/tasks_helper.rb b/app/helpers/tasks_helper.rb index d8e0780e4c1..4498cd3af25 100644 --- a/app/helpers/tasks_helper.rb +++ b/app/helpers/tasks_helper.rb @@ -24,9 +24,7 @@ module TasksHelper end def task_target_link_html(task) - link_to task_target_path(task) do - "##{task.target_iid}" - end + link_to "##{task.target_iid}", task_target_path(task) end def task_target_path(task) @@ -36,11 +34,6 @@ module TasksHelper task.project, task.target], anchor: anchor) end - def task_body(text, options = {}) - text = first_line_in_markdown(text, 150, options) - sanitize(text, tags: %w(a img b pre code p span)) - end - def task_actions_options actions = [ OpenStruct.new(id: '', title: 'Any Action'), diff --git a/app/views/dashboard/tasks/_task.html.haml b/app/views/dashboard/tasks/_task.html.haml index d08b021f53b..164a53dd928 100644 --- a/app/views/dashboard/tasks/_task.html.haml +++ b/app/views/dashboard/tasks/_task.html.haml @@ -18,4 +18,4 @@ .task-body .task-note .md - = task_body(task.body, project: task.project) + = event_note(task.body, project: task.project) -- 2.18.1 From 408e010d65e7e2e2b64a694e12d44636d7d81dec Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Sat, 20 Feb 2016 10:35:36 -0200 Subject: [PATCH 41/50] Rename IssuableBaseService#have_changes? to has_changes? --- app/services/issuable_base_service.rb | 2 +- app/services/issues/update_service.rb | 2 +- app/services/merge_requests/update_service.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index fef96639ace..ca87dca4a70 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -71,7 +71,7 @@ class IssuableBaseService < BaseService end end - def have_changes?(issuable, options = {}) + def has_changes?(issuable, options = {}) valid_attrs = [:title, :description, :assignee_id, :milestone_id, :target_branch] attrs_changed = valid_attrs.any? do |attr| diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index d2620750305..4d85f6464b1 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -5,7 +5,7 @@ module Issues end def handle_changes(issue, options = {}) - if have_changes?(issue, options) + if has_changes?(issue, options) task_service.mark_pending_tasks_as_done(issue, current_user) end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index a89daf9821e..b9781864697 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -15,7 +15,7 @@ module MergeRequests end def handle_changes(merge_request, options = {}) - if have_changes?(merge_request, options) + if has_changes?(merge_request, options) task_service.mark_pending_tasks_as_done(merge_request, current_user) end -- 2.18.1 From 3d52e139b13ad077286f2f9f46b7e98f43ad9564 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Sat, 20 Feb 2016 11:59:59 -0200 Subject: [PATCH 42/50] Rename Tasks to Todos --- CHANGELOG | 4 +- .../pages/{tasks.scss => todos.scss} | 26 +- app/controllers/dashboard/tasks_controller.rb | 21 -- app/controllers/dashboard/todos_controller.rb | 23 ++ .../projects/merge_requests_controller.rb | 2 +- .../{tasks_finder.rb => todos_finder.rb} | 10 +- app/helpers/tasks_helper.rb | 69 ----- app/helpers/todos_helper.rb | 59 ++++ app/models/note.rb | 2 +- app/models/{task.rb => todo.rb} | 4 +- app/models/user.rb | 2 +- app/services/base_service.rb | 4 +- app/services/issues/close_service.rb | 4 +- app/services/issues/create_service.rb | 2 +- app/services/issues/update_service.rb | 6 +- app/services/merge_requests/close_service.rb | 2 +- app/services/merge_requests/create_service.rb | 2 +- app/services/merge_requests/update_service.rb | 6 +- app/services/notes/create_service.rb | 2 +- app/services/notes/update_service.rb | 2 +- .../{task_service.rb => todo_service.rb} | 86 +++--- app/views/dashboard/tasks/_task.html.haml | 21 -- app/views/dashboard/todos/_todo.html.haml | 21 ++ .../{tasks => todos}/index.html.haml | 28 +- app/views/layouts/header/_default.html.haml | 6 +- app/views/layouts/nav/_dashboard.html.haml | 8 +- config/routes.rb | 2 +- .../20160220123949_rename_tasks_to_todos.rb | 5 + db/schema.rb | 16 +- .../{tasks.feature => todos.feature} | 22 +- .../steps/dashboard/{tasks.rb => todos.rb} | 64 ++--- features/steps/shared/paths.rb | 4 +- spec/factories/{tasks.rb => todos.rb} | 10 +- spec/models/note_spec.rb | 2 +- spec/models/{task_spec.rb => todo_spec.rb.rb} | 8 +- spec/models/user_spec.rb | 2 +- spec/services/issues/close_service_spec.rb | 8 +- spec/services/issues/create_service_spec.rb | 9 +- spec/services/issues/update_service_spec.rb | 33 +-- .../merge_requests/close_service_spec.rb | 6 +- .../merge_requests/create_service_spec.rb | 16 +- .../merge_requests/update_service_spec.rb | 37 +-- spec/services/notes/update_service_spec.rb | 12 +- spec/services/task_service_spec.rb | 264 ------------------ spec/services/todo_service_spec.rb | 264 ++++++++++++++++++ 45 files changed, 604 insertions(+), 602 deletions(-) rename app/assets/stylesheets/pages/{tasks.scss => todos.scss} (87%) delete mode 100644 app/controllers/dashboard/tasks_controller.rb create mode 100644 app/controllers/dashboard/todos_controller.rb rename app/finders/{tasks_finder.rb => todos_finder.rb} (92%) delete mode 100644 app/helpers/tasks_helper.rb create mode 100644 app/helpers/todos_helper.rb rename app/models/{task.rb => todo.rb} (96%) rename app/services/{task_service.rb => todo_service.rb} (50%) delete mode 100644 app/views/dashboard/tasks/_task.html.haml create mode 100644 app/views/dashboard/todos/_todo.html.haml rename app/views/dashboard/{tasks => todos}/index.html.haml (76%) create mode 100644 db/migrate/20160220123949_rename_tasks_to_todos.rb rename features/dashboard/{tasks.feature => todos.feature} (58%) rename features/steps/dashboard/{tasks.rb => todos.rb} (52%) rename spec/factories/{tasks.rb => todos.rb} (81%) rename spec/models/{task_spec.rb => todo_spec.rb.rb} (94%) delete mode 100644 spec/services/task_service_spec.rb create mode 100644 spec/services/todo_service_spec.rb diff --git a/CHANGELOG b/CHANGELOG index 1225ce42683..d8d40239db2 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -65,7 +65,7 @@ v 8.5.0 (unreleased) - Fix broken link to project in build notification emails - Ability to see and sort on vote count from Issues and MR lists - Fix builds scheduler when first build in stage was allowed to fail - - User project limit is reached notice is hidden if the projects limit is zero + - User project limit is reached notice is hidden if the projects limit is zero - Add API support for managing runners and project's runners - Allow SAML users to login with no previous account without having to allow all Omniauth providers to do so. @@ -75,7 +75,7 @@ v 8.5.0 (unreleased) - Emoji comment on diffs are not award emoji - Add label description (Nuttanart Pornprasitsakul) - Show label row when filtering issues or merge requests by label (Nuttanart Pornprasitsakul) - - Add Task Queue + - Add Todos v 8.4.4 - Update omniauth-saml gem to 1.4.2 diff --git a/app/assets/stylesheets/pages/tasks.scss b/app/assets/stylesheets/pages/todos.scss similarity index 87% rename from app/assets/stylesheets/pages/tasks.scss rename to app/assets/stylesheets/pages/todos.scss index a3dffeed4ab..2f57f21963d 100644 --- a/app/assets/stylesheets/pages/tasks.scss +++ b/app/assets/stylesheets/pages/todos.scss @@ -1,37 +1,37 @@ /** - * Dashboard tasks queue + * Dashboard Todos * */ .navbar-nav { li { - .badge.tasks-pending-count { + .badge.todos-pending-count { background-color: #7f8fa4; margin-top: -5px; } } } -.tasks { +.todos { .panel { border-top: none; margin-bottom: 0; } } -.task-item { +.todo-item { font-size: $gl-font-size; padding: $gl-padding-top 0 $gl-padding-top ($gl-avatar-size + $gl-padding-top); border-bottom: 1px solid $table-border-color; color: #7f8fa4; - &.task-inline { + &.todo-inline { .avatar { position: relative; top: -2px; } - .task-title { + .todo-title { line-height: 40px; } } @@ -44,7 +44,7 @@ margin-left: -($gl-avatar-size + $gl-padding-top); } - .task-title { + .todo-title { @include str-truncated(calc(100% - 174px)); font-weight: 600; @@ -53,10 +53,10 @@ } } - .task-body { + .todo-body { margin-right: 174px; - .task-note { + .todo-note { word-wrap: break-word; .md { @@ -89,7 +89,7 @@ } } - .task-note-icon { + .todo-note-icon { color: #777; float: left; font-size: $gl-font-size; @@ -102,10 +102,10 @@ } @media (max-width: $screen-xs-max) { - .task-item { + .todo-item { padding-left: $gl-padding; - .task-title { + .todo-title { white-space: normal; overflow: visible; max-width: 100%; @@ -115,7 +115,7 @@ display: none; } - .task-body { + .todo-body { margin: 0; border-left: 2px solid #DDD; padding-left: 10px; diff --git a/app/controllers/dashboard/tasks_controller.rb b/app/controllers/dashboard/tasks_controller.rb deleted file mode 100644 index a8884be54e4..00000000000 --- a/app/controllers/dashboard/tasks_controller.rb +++ /dev/null @@ -1,21 +0,0 @@ -class Dashboard::TasksController < Dashboard::ApplicationController - def index - @tasks = TasksFinder.new(current_user, params).execute - @tasks = @tasks.page(params[:page]).per(PER_PAGE) - end - - def destroy - task.done! - - respond_to do |format| - format.html { redirect_to dashboard_tasks_path, notice: 'Task was successfully marked as done.' } - format.js { render nothing: true } - end - end - - private - - def task - @task ||= current_user.tasks.find(params[:id]) - end -end diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb new file mode 100644 index 00000000000..e24b8d3e58f --- /dev/null +++ b/app/controllers/dashboard/todos_controller.rb @@ -0,0 +1,23 @@ +class Dashboard::TodosController < Dashboard::ApplicationController + def index + @todos = TodosFinder.new(current_user, params).execute + @todos = @todos.page(params[:page]).per(PER_PAGE) + end + + def destroy + todo.done! + + respond_to do |format| + format.html { redirect_to dashboard_todos_path, notice: 'Todo was successfully marked as done.' } + format.js { render nothing: true } + end + end + + private + + def todo + @todo ||= current_user.todos.find(params[:id]) + end +end + + diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index be58156a9dc..5fe21694605 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -181,7 +181,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController return end - TaskService.new.merge_merge_request(merge_request, current_user) + TodoService.new.merge_merge_request(merge_request, current_user) @merge_request.update(merge_error: nil) diff --git a/app/finders/tasks_finder.rb b/app/finders/todos_finder.rb similarity index 92% rename from app/finders/tasks_finder.rb rename to app/finders/todos_finder.rb index 2a32e977c24..3ba27c40504 100644 --- a/app/finders/tasks_finder.rb +++ b/app/finders/todos_finder.rb @@ -1,6 +1,6 @@ -# TasksFinder +# TodosFinder # -# Used to filter Tasks by set of params +# Used to filter Todos by set of params # # Arguments: # current_user - which user use @@ -12,7 +12,7 @@ # type: 'Issue' or 'MergeRequest' # -class TasksFinder +class TodosFinder NONE = '0' attr_accessor :current_user, :params @@ -23,7 +23,7 @@ class TasksFinder end def execute - items = current_user.tasks + items = current_user.todos items = by_action_id(items) items = by_author(items) items = by_project(items) @@ -36,7 +36,7 @@ class TasksFinder private def action_id? - action_id.present? && [Task::ASSIGNED, Task::MENTIONED].include?(action_id.to_i) + action_id.present? && [Todo::ASSIGNED, Todo::MENTIONED].include?(action_id.to_i) end def action_id diff --git a/app/helpers/tasks_helper.rb b/app/helpers/tasks_helper.rb deleted file mode 100644 index 4498cd3af25..00000000000 --- a/app/helpers/tasks_helper.rb +++ /dev/null @@ -1,69 +0,0 @@ -module TasksHelper - def link_to_author(task) - author = task.author - - if author - link_to author.name, user_path(author.username) - else - task.author_name - end - end - - def tasks_pending_count - current_user.tasks.pending.count - end - - def tasks_done_count - current_user.tasks.done.count - end - - def task_action_name(task) - target = task.target_type.titleize.downcase - - [task.action_name, target].join(" ") - end - - def task_target_link_html(task) - link_to "##{task.target_iid}", task_target_path(task) - end - - def task_target_path(task) - anchor = dom_id(task.note) if task.note.present? - - polymorphic_path([task.project.namespace.becomes(Namespace), - task.project, task.target], anchor: anchor) - end - - def task_actions_options - actions = [ - OpenStruct.new(id: '', title: 'Any Action'), - OpenStruct.new(id: Task::ASSIGNED, title: 'Assigned'), - OpenStruct.new(id: Task::MENTIONED, title: 'Mentioned') - ] - - options_from_collection_for_select(actions, 'id', 'title', params[:action_id]) - end - - def task_projects_options - projects = current_user.authorized_projects.sorted_by_activity.non_archived - projects = projects.includes(:namespace) - - projects = projects.map do |project| - OpenStruct.new(id: project.id, title: project.name_with_namespace) - end - - projects.unshift(OpenStruct.new(id: '', title: 'Any Project')) - - options_from_collection_for_select(projects, 'id', 'title', params[:project_id]) - end - - def task_types_options - types = [ - OpenStruct.new(title: 'Any Type', name: ''), - OpenStruct.new(title: 'Issue', name: 'Issue'), - OpenStruct.new(title: 'Merge Request', name: 'MergeRequest') - ] - - options_from_collection_for_select(types, 'name', 'title', params[:type]) - end -end diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb new file mode 100644 index 00000000000..7c360468945 --- /dev/null +++ b/app/helpers/todos_helper.rb @@ -0,0 +1,59 @@ +module TodosHelper + def todos_pending_count + current_user.todos.pending.count + end + + def todos_done_count + current_user.todos.done.count + end + + def todo_action_name(todo) + target = todo.target_type.titleize.downcase + + [todo.action_name, target].join(" ") + end + + def todo_target_link_html(todo) + link_to "##{todo.target_iid}", todo_target_path(todo) + end + + def todo_target_path(todo) + anchor = dom_id(todo.note) if todo.note.present? + + polymorphic_path([todo.project.namespace.becomes(Namespace), + todo.project, todo.target], anchor: anchor) + end + + def todo_actions_options + actions = [ + OpenStruct.new(id: '', title: 'Any Action'), + OpenStruct.new(id: Todo::ASSIGNED, title: 'Assigned'), + OpenStruct.new(id: Todo::MENTIONED, title: 'Mentioned') + ] + + options_from_collection_for_select(actions, 'id', 'title', params[:action_id]) + end + + def todo_projects_options + projects = current_user.authorized_projects.sorted_by_activity.non_archived + projects = projects.includes(:namespace) + + projects = projects.map do |project| + OpenStruct.new(id: project.id, title: project.name_with_namespace) + end + + projects.unshift(OpenStruct.new(id: '', title: 'Any Project')) + + options_from_collection_for_select(projects, 'id', 'title', params[:project_id]) + end + + def todo_types_options + types = [ + OpenStruct.new(title: 'Any Type', name: ''), + OpenStruct.new(title: 'Issue', name: 'Issue'), + OpenStruct.new(title: 'Merge Request', name: 'MergeRequest') + ] + + options_from_collection_for_select(types, 'name', 'title', params[:type]) + end +end diff --git a/app/models/note.rb b/app/models/note.rb index 31606cf8222..d287e0f3c6d 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -37,7 +37,7 @@ class Note < ActiveRecord::Base belongs_to :author, class_name: "User" belongs_to :updated_by, class_name: "User" - has_many :tasks, dependent: :destroy + has_many :todos, dependent: :destroy delegate :name, to: :project, prefix: true delegate :name, :email, to: :author, prefix: true diff --git a/app/models/task.rb b/app/models/todo.rb similarity index 96% rename from app/models/task.rb rename to app/models/todo.rb index 0872743097c..7a9b0212f9f 100644 --- a/app/models/task.rb +++ b/app/models/todo.rb @@ -1,6 +1,6 @@ # == Schema Information # -# Table name: tasks +# Table name: todos # # id :integer not null, primary key # user_id :integer not null @@ -15,7 +15,7 @@ # updated_at :datetime # -class Task < ActiveRecord::Base +class Todo < ActiveRecord::Base ASSIGNED = 1 MENTIONED = 2 diff --git a/app/models/user.rb b/app/models/user.rb index d108ba78e4b..02ff2456f2b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -140,7 +140,7 @@ class User < ActiveRecord::Base has_one :abuse_report, dependent: :destroy has_many :spam_logs, dependent: :destroy has_many :builds, dependent: :nullify, class_name: 'Ci::Build' - has_many :tasks, dependent: :destroy + has_many :todos, dependent: :destroy # # Validations diff --git a/app/services/base_service.rb b/app/services/base_service.rb index c349997b9e4..8563633816c 100644 --- a/app/services/base_service.rb +++ b/app/services/base_service.rb @@ -23,8 +23,8 @@ class BaseService EventCreateService.new end - def task_service - TaskService.new + def todo_service + TodoService.new end def log_info(message) diff --git a/app/services/issues/close_service.rb b/app/services/issues/close_service.rb index a652bba4761..78254b49af3 100644 --- a/app/services/issues/close_service.rb +++ b/app/services/issues/close_service.rb @@ -3,7 +3,7 @@ module Issues def execute(issue, commit = nil) if project.jira_tracker? && project.jira_service.active project.jira_service.execute(commit, issue) - task_service.close_issue(issue, current_user) + todo_service.close_issue(issue, current_user) return issue end @@ -11,7 +11,7 @@ module Issues event_service.close_issue(issue, current_user) create_note(issue, commit) notification_service.close_issue(issue, current_user) - task_service.close_issue(issue, current_user) + todo_service.close_issue(issue, current_user) execute_hooks(issue, 'close') end diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 2a6c84c3ce5..10787e8873c 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -9,7 +9,7 @@ module Issues if issue.save issue.update_attributes(label_ids: label_params) notification_service.new_issue(issue, current_user) - task_service.new_issue(issue, current_user) + todo_service.new_issue(issue, current_user) event_service.open_issue(issue, current_user) issue.create_cross_references!(current_user) execute_hooks(issue, 'open') diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index 4d85f6464b1..51ef9dfe610 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -6,12 +6,12 @@ module Issues def handle_changes(issue, options = {}) if has_changes?(issue, options) - task_service.mark_pending_tasks_as_done(issue, current_user) + todo_service.mark_pending_todos_as_done(issue, current_user) end if issue.previous_changes.include?('title') || issue.previous_changes.include?('description') - task_service.update_issue(issue, current_user) + todo_service.update_issue(issue, current_user) end if issue.previous_changes.include?('milestone_id') @@ -21,7 +21,7 @@ module Issues if issue.previous_changes.include?('assignee_id') create_assignee_note(issue) notification_service.reassigned_issue(issue, current_user) - task_service.reassigned_issue(issue, current_user) + todo_service.reassigned_issue(issue, current_user) end end diff --git a/app/services/merge_requests/close_service.rb b/app/services/merge_requests/close_service.rb index 1f70c95ab4a..27ee81fe3e7 100644 --- a/app/services/merge_requests/close_service.rb +++ b/app/services/merge_requests/close_service.rb @@ -9,7 +9,7 @@ module MergeRequests event_service.close_mr(merge_request, current_user) create_note(merge_request) notification_service.close_mr(merge_request, current_user) - task_service.close_merge_request(merge_request, current_user) + todo_service.close_merge_request(merge_request, current_user) execute_hooks(merge_request, 'close') end diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index b5691cdf44f..33609d01f20 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -18,7 +18,7 @@ module MergeRequests merge_request.update_attributes(label_ids: label_params) event_service.open_mr(merge_request, current_user) notification_service.new_merge_request(merge_request, current_user) - task_service.new_merge_request(merge_request, current_user) + todo_service.new_merge_request(merge_request, current_user) merge_request.create_cross_references!(current_user) execute_hooks(merge_request) end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index b9781864697..6319ad805b6 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -16,12 +16,12 @@ module MergeRequests def handle_changes(merge_request, options = {}) if has_changes?(merge_request, options) - task_service.mark_pending_tasks_as_done(merge_request, current_user) + todo_service.mark_pending_todos_as_done(merge_request, current_user) end if merge_request.previous_changes.include?('title') || merge_request.previous_changes.include?('description') - task_service.update_merge_request(merge_request, current_user) + todo_service.update_merge_request(merge_request, current_user) end if merge_request.previous_changes.include?('target_branch') @@ -37,7 +37,7 @@ module MergeRequests if merge_request.previous_changes.include?('assignee_id') create_assignee_note(merge_request) notification_service.reassigned_merge_request(merge_request, current_user) - task_service.reassigned_merge_request(merge_request, current_user) + todo_service.reassigned_merge_request(merge_request, current_user) end if merge_request.previous_changes.include?('target_branch') || diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index dbbf9e63164..b970439b921 100644 --- a/app/services/notes/create_service.rb +++ b/app/services/notes/create_service.rb @@ -8,7 +8,7 @@ module Notes if note.save # Finish the harder work in the background NewNoteWorker.perform_in(2.seconds, note.id, params) - TaskService.new.new_note(note, current_user) + TodoService.new.new_note(note, current_user) end note diff --git a/app/services/notes/update_service.rb b/app/services/notes/update_service.rb index 6c2d36546e1..1361b1e0300 100644 --- a/app/services/notes/update_service.rb +++ b/app/services/notes/update_service.rb @@ -8,7 +8,7 @@ module Notes note.reset_events_cache if note.previous_changes.include?('note') - TaskService.new.update_note(note, current_user) + TodoService.new.update_note(note, current_user) end note diff --git a/app/services/task_service.rb b/app/services/todo_service.rb similarity index 50% rename from app/services/task_service.rb rename to app/services/todo_service.rb index c4479fe6382..dc270602ebc 100644 --- a/app/services/task_service.rb +++ b/app/services/todo_service.rb @@ -1,15 +1,15 @@ -# TaskService class +# TodoService class # -# Used for creating tasks on task queue after certain user action +# Used for creating todos after certain user actions # # Ex. -# TaskService.new.new_issue(issue, current_user) +# TodoService.new.new_issue(issue, current_user) # -class TaskService +class TodoService # When create an issue we should: # - # * create a task for assignee if issue is assigned - # * create a task for each mentioned user on issue + # * create a todo for assignee if issue is assigned + # * create a todo for each mentioned user on issue # def new_issue(issue, current_user) new_issuable(issue, current_user) @@ -17,32 +17,32 @@ class TaskService # When update an issue we should: # - # * mark all pending tasks related to the issue for the current user as done + # * mark all pending todos related to the issue for the current user as done # def update_issue(issue, current_user) - create_mention_tasks(issue.project, issue, current_user) + create_mention_todos(issue.project, issue, current_user) end # When close an issue we should: # - # * mark all pending tasks related to the target for the current user as done + # * mark all pending todos related to the target for the current user as done # def close_issue(issue, current_user) - mark_pending_tasks_as_done(issue, current_user) + mark_pending_todos_as_done(issue, current_user) end # When we reassign an issue we should: # - # * create a pending task for new assignee if issue is assigned + # * create a pending todo for new assignee if issue is assigned # def reassigned_issue(issue, current_user) - create_assignment_task(issue, current_user) + create_assignment_todo(issue, current_user) end # When create a merge request we should: # - # * creates a pending task for assignee if merge request is assigned - # * create a task for each mentioned user on merge request + # * creates a pending todo for assignee if merge request is assigned + # * create a todo for each mentioned user on merge request # def new_merge_request(merge_request, current_user) new_issuable(merge_request, current_user) @@ -50,40 +50,40 @@ class TaskService # When update a merge request we should: # - # * create a task for each mentioned user on merge request + # * create a todo for each mentioned user on merge request # def update_merge_request(merge_request, current_user) - create_mention_tasks(merge_request.project, merge_request, current_user) + create_mention_todos(merge_request.project, merge_request, current_user) end # When close a merge request we should: # - # * mark all pending tasks related to the target for the current user as done + # * mark all pending todos related to the target for the current user as done # def close_merge_request(merge_request, current_user) - mark_pending_tasks_as_done(merge_request, current_user) + mark_pending_todos_as_done(merge_request, current_user) end # When we reassign a merge request we should: # - # * creates a pending task for new assignee if merge request is assigned + # * creates a pending todo for new assignee if merge request is assigned # def reassigned_merge_request(merge_request, current_user) - create_assignment_task(merge_request, current_user) + create_assignment_todo(merge_request, current_user) end # When merge a merge request we should: # - # * mark all pending tasks related to the target for the current user as done + # * mark all pending todos related to the target for the current user as done # def merge_merge_request(merge_request, current_user) - mark_pending_tasks_as_done(merge_request, current_user) + mark_pending_todos_as_done(merge_request, current_user) end # When create a note we should: # - # * mark all pending tasks related to the noteable for the note author as done - # * create a task for each mentioned user on note + # * mark all pending todos related to the noteable for the note author as done + # * create a todo for each mentioned user on note # def new_note(note, current_user) handle_note(note, current_user) @@ -91,28 +91,28 @@ class TaskService # When update a note we should: # - # * mark all pending tasks related to the noteable for the current user as done - # * create a task for each new user mentioned on note + # * mark all pending todos related to the noteable for the current user as done + # * create a todo for each new user mentioned on note # def update_note(note, current_user) handle_note(note, current_user) end - # When marking pending tasks as done we should: + # When marking pending todos as done we should: # - # * mark all pending tasks related to the target for the current user as done + # * mark all pending todos related to the target for the current user as done # - def mark_pending_tasks_as_done(target, user) - pending_tasks(user, target.project, target).update_all(state: :done) + def mark_pending_todos_as_done(target, user) + pending_todos(user, target.project, target).update_all(state: :done) end private - def create_tasks(project, target, author, users, action, note = nil) + def create_todos(project, target, author, users, action, note = nil) Array(users).each do |user| - next if pending_tasks(user, project, target).exists? + next if pending_todos(user, project, target).exists? - Task.create( + Todo.create( project: project, user_id: user.id, author_id: author.id, @@ -125,8 +125,8 @@ class TaskService end def new_issuable(issuable, author) - create_assignment_task(issuable, author) - create_mention_tasks(issuable.project, issuable, author) + create_assignment_todo(issuable, author) + create_mention_todos(issuable.project, issuable, author) end def handle_note(note, author) @@ -136,19 +136,19 @@ class TaskService project = note.project target = note.noteable - mark_pending_tasks_as_done(target, author) - create_mention_tasks(project, target, author, note) + mark_pending_todos_as_done(target, author) + create_mention_todos(project, target, author, note) end - def create_assignment_task(issuable, author) + def create_assignment_todo(issuable, author) if issuable.assignee && issuable.assignee != author - create_tasks(issuable.project, issuable, author, issuable.assignee, Task::ASSIGNED) + create_todos(issuable.project, issuable, author, issuable.assignee, Todo::ASSIGNED) end end - def create_mention_tasks(project, issuable, author, note = nil) + def create_mention_todos(project, issuable, author, note = nil) mentioned_users = filter_mentioned_users(project, note || issuable, author) - create_tasks(project, issuable, author, mentioned_users, Task::MENTIONED, note) + create_todos(project, issuable, author, mentioned_users, Todo::MENTIONED, note) end def filter_mentioned_users(project, target, author) @@ -160,8 +160,8 @@ class TaskService mentioned_users.uniq end - def pending_tasks(user, project, target) - user.tasks.pending.where( + def pending_todos(user, project, target) + user.todos.pending.where( project_id: project.id, target_id: target.id, target_type: target.class.name diff --git a/app/views/dashboard/tasks/_task.html.haml b/app/views/dashboard/tasks/_task.html.haml deleted file mode 100644 index 164a53dd928..00000000000 --- a/app/views/dashboard/tasks/_task.html.haml +++ /dev/null @@ -1,21 +0,0 @@ -%li{class: "task task-#{task.done? ? 'done' : 'pending'}", id: dom_id(task) } - .task-item{class: 'task-block'} - = image_tag avatar_icon(task.author_email, 40), class: 'avatar s40', alt:'' - - .task-title - %span.author_name - = link_to_author task - %span.task_label - = task_action_name(task) - = task_target_link_html(task) - - · #{time_ago_with_tooltip(task.created_at)} - - - if task.pending? - .task-actions.pull-right - = link_to 'Done', [:dashboard, task], method: :delete, class: 'btn' - - .task-body - .task-note - .md - = event_note(task.body, project: task.project) diff --git a/app/views/dashboard/todos/_todo.html.haml b/app/views/dashboard/todos/_todo.html.haml new file mode 100644 index 00000000000..7de703e304f --- /dev/null +++ b/app/views/dashboard/todos/_todo.html.haml @@ -0,0 +1,21 @@ +%li{class: "todo todo-#{todo.done? ? 'done' : 'pending'}", id: dom_id(todo) } + .todo-item{class: 'todo-block'} + = image_tag avatar_icon(todo.author_email, 40), class: 'avatar s40', alt:'' + + .todo-title + %span.author_name + = link_to_author todo + %span.todo_label + = todo_action_name(todo) + = todo_target_link_html(todo) + + · #{time_ago_with_tooltip(todo.created_at)} + + - if todo.pending? + .todo-actions.pull-right + = link_to 'Done', [:dashboard, todo], method: :delete, class: 'btn' + + .todo-body + .todo-note + .md + = event_note(todo.body, project: todo.project) diff --git a/app/views/dashboard/tasks/index.html.haml b/app/views/dashboard/todos/index.html.haml similarity index 76% rename from app/views/dashboard/tasks/index.html.haml rename to app/views/dashboard/todos/index.html.haml index 4b6e3d83e62..a91f8f24f8c 100644 --- a/app/views/dashboard/tasks/index.html.haml +++ b/app/views/dashboard/todos/index.html.haml @@ -1,53 +1,53 @@ -- page_title "Tasks" -- header_title "Tasks", dashboard_tasks_path +- page_title "Todos" +- header_title "Todos", dashboard_todos_path .top-area %ul.nav-links %li{class: ('active' if params[:state].blank? || params[:state] == 'pending')} = link_to page_filter_path(state: 'pending') do %span - Tasks + Todos %span{class: 'badge'} - = tasks_pending_count + = todos_pending_count %li{class: ('active' if params[:state] == 'done')} = link_to page_filter_path(state: 'done') do %span Done %span{class: 'badge'} - = tasks_done_count + = todos_done_count -.tasks-filters +.todos-filters .gray-content-block.second-block = form_tag page_filter_path(without: [:assignee_id, :milestone_title, :label_name, :scope, :sort]), method: :get, class: 'filter-form' do .filter-item.inline - = select_tag('project_id', task_projects_options, + = select_tag('project_id', todo_projects_options, class: 'select2 trigger-submit', include_blank: true, data: {placeholder: 'Project'}) .filter-item.inline = users_select_tag(:author_id, selected: params[:author_id], placeholder: 'Author', class: 'trigger-submit', any_user: "Any Author", first_user: true, current_user: true) .filter-item.inline - = select_tag('type', task_types_options, + = select_tag('type', todo_types_options, class: 'select2 trigger-submit', include_blank: true, data: {placeholder: 'Type'}) .filter-item.inline.actions-filter - = select_tag('action_id', task_actions_options, + = select_tag('action_id', todo_actions_options, class: 'select2 trigger-submit', include_blank: true, data: {placeholder: 'Action'}) .prepend-top-default - - if @tasks.any? - - @tasks.group_by(&:project).each do |group| + - if @todos.any? + - @todos.group_by(&:project).each do |group| .panel.panel-default.panel-small - project = group[0] .panel-heading = link_to project.name_with_namespace, namespace_project_path(project.namespace, project) - %ul.well-list.tasks-list + %ul.well-list.todos-list = render group[1] - = paginate @tasks, theme: "gitlab" + = paginate @todos, theme: "gitlab" - else - .nothing-here-block No tasks to show + .nothing-here-block No todos to show :javascript new UsersSelect(); diff --git a/app/views/layouts/header/_default.html.haml b/app/views/layouts/header/_default.html.haml index 3d55c4bba1b..4781ff23507 100644 --- a/app/views/layouts/header/_default.html.haml +++ b/app/views/layouts/header/_default.html.haml @@ -22,9 +22,9 @@ = link_to admin_root_path, title: 'Admin Area', data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do = icon('wrench fw') %li - = link_to dashboard_tasks_path, title: 'Tasks', data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do - %span.badge.tasks-pending-count - = tasks_pending_count + = link_to dashboard_todos_path, title: 'Todos', data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do + %span.badge.todos-pending-count + = todos_pending_count - if current_user.can_create_project? %li = link_to new_project_path, title: 'New project', data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do diff --git a/app/views/layouts/nav/_dashboard.html.haml b/app/views/layouts/nav/_dashboard.html.haml index 586eff002df..db0cf393922 100644 --- a/app/views/layouts/nav/_dashboard.html.haml +++ b/app/views/layouts/nav/_dashboard.html.haml @@ -4,12 +4,12 @@ = icon('home fw') %span Projects - = nav_link(controller: :tasks) do - = link_to dashboard_tasks_path, title: 'Tasks' do + = nav_link(controller: :todos) do + = link_to dashboard_todos_path, title: 'Todos' do = icon('bell fw') %span - Tasks - %span.count= number_with_delimiter(tasks_pending_count) + Todos + %span.count= number_with_delimiter(todos_pending_count) = nav_link(path: 'dashboard#activity') do = link_to activity_dashboard_path, class: 'shortcuts-activity', title: 'Activity' do = icon('dashboard fw') diff --git a/config/routes.rb b/config/routes.rb index 0b263933fba..c228a51bed1 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -333,7 +333,7 @@ Rails.application.routes.draw do resources :groups, only: [:index] resources :snippets, only: [:index] - resources :tasks, only: [:index, :destroy] + resources :todos, only: [:index, :destroy] resources :projects, only: [:index] do collection do diff --git a/db/migrate/20160220123949_rename_tasks_to_todos.rb b/db/migrate/20160220123949_rename_tasks_to_todos.rb new file mode 100644 index 00000000000..30c10d27146 --- /dev/null +++ b/db/migrate/20160220123949_rename_tasks_to_todos.rb @@ -0,0 +1,5 @@ +class RenameTasksToTodos < ActiveRecord::Migration + def change + rename_table :tasks, :todos + end +end diff --git a/db/schema.rb b/db/schema.rb index 2b726d17682..4708c29d9ae 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160217174422) do +ActiveRecord::Schema.define(version: 20160220123949) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -824,7 +824,7 @@ ActiveRecord::Schema.define(version: 20160217174422) do add_index "tags", ["name"], name: "index_tags_on_name", unique: true, using: :btree - create_table "tasks", force: :cascade do |t| + create_table "todos", force: :cascade do |t| t.integer "user_id", null: false t.integer "project_id", null: false t.integer "target_id", null: false @@ -837,12 +837,12 @@ ActiveRecord::Schema.define(version: 20160217174422) do t.integer "note_id" end - add_index "tasks", ["author_id"], name: "index_tasks_on_author_id", using: :btree - add_index "tasks", ["note_id"], name: "index_tasks_on_note_id", using: :btree - add_index "tasks", ["project_id"], name: "index_tasks_on_project_id", using: :btree - add_index "tasks", ["state"], name: "index_tasks_on_state", using: :btree - add_index "tasks", ["target_type", "target_id"], name: "index_tasks_on_target_type_and_target_id", using: :btree - add_index "tasks", ["user_id"], name: "index_tasks_on_user_id", using: :btree + add_index "todos", ["author_id"], name: "index_todos_on_author_id", using: :btree + add_index "todos", ["note_id"], name: "index_todos_on_note_id", using: :btree + add_index "todos", ["project_id"], name: "index_todos_on_project_id", using: :btree + add_index "todos", ["state"], name: "index_todos_on_state", using: :btree + add_index "todos", ["target_type", "target_id"], name: "index_todos_on_target_type_and_target_id", using: :btree + add_index "todos", ["user_id"], name: "index_todos_on_user_id", using: :btree create_table "users", force: :cascade do |t| t.string "email", default: "", null: false diff --git a/features/dashboard/tasks.feature b/features/dashboard/todos.feature similarity index 58% rename from features/dashboard/tasks.feature rename to features/dashboard/todos.feature index 9d4f386f7dd..1e7b1b50d64 100644 --- a/features/dashboard/tasks.feature +++ b/features/dashboard/todos.feature @@ -1,5 +1,5 @@ @dashboard -Feature: Dashboard Tasks +Feature: Dashboard Todos Background: Given I sign in as a user And I own project "Shop" @@ -7,32 +7,32 @@ Feature: Dashboard Tasks And "Mary Jane" is a developer of project "Shop" And "Mary Jane" owns private project "Enterprise" And I am a developer of project "Enterprise" - And I have pending tasks - And I visit dashboard task queue page + And I have todos + And I visit dashboard todos page @javascript - Scenario: I mark pending tasks as done - Then I should see pending tasks assigned to me - And I mark the pending task as done + Scenario: I mark todos as done + Then I should see todos assigned to me + And I mark the todo as done And I click on the "Done" tab - Then I should see all tasks marked as done + Then I should see all todos marked as done @javascript Scenario: I filter by project Given I filter by "Enterprise" - Then I should not see tasks + Then I should not see todos @javascript Scenario: I filter by author Given I filter by "John Doe" - Then I should not see tasks related to "Mary Jane" in the list + Then I should not see todos related to "Mary Jane" in the list @javascript Scenario: I filter by type Given I filter by "Issue" - Then I should not see tasks related to "Merge Requests" in the list + Then I should not see todos related to "Merge Requests" in the list @javascript Scenario: I filter by action Given I filter by "Mentioned" - Then I should not see tasks related to "Assignments" in the list + Then I should not see todos related to "Assignments" in the list diff --git a/features/steps/dashboard/tasks.rb b/features/steps/dashboard/todos.rb similarity index 52% rename from features/steps/dashboard/tasks.rb rename to features/steps/dashboard/todos.rb index 556aa41bad9..207d997f8a0 100644 --- a/features/steps/dashboard/tasks.rb +++ b/features/steps/dashboard/todos.rb @@ -1,4 +1,4 @@ -class Spinach::Features::DashboardTasks < Spinach::FeatureSteps +class Spinach::Features::DashboardTodos < Spinach::FeatureSteps include SharedAuthentication include SharedPaths include SharedProject @@ -17,43 +17,43 @@ class Spinach::Features::DashboardTasks < Spinach::FeatureSteps project.team << [john_doe, :developer] end - step 'I have pending tasks' do - create(:task, user: current_user, project: project, author: mary_jane, target: issue, action: Task::MENTIONED) - create(:task, user: current_user, project: project, author: john_doe, target: issue, action: Task::ASSIGNED) + step 'I have todos' do + create(:todo, user: current_user, project: project, author: mary_jane, target: issue, action: Todo::MENTIONED) + create(:todo, user: current_user, project: project, author: john_doe, target: issue, action: Todo::ASSIGNED) note = create(:note, author: john_doe, noteable: issue, note: "#{current_user.to_reference} Wdyt?") - create(:task, user: current_user, project: project, author: john_doe, target: issue, action: Task::MENTIONED, note: note) - create(:task, user: current_user, project: project, author: john_doe, target: merge_request, action: Task::ASSIGNED) + create(:todo, user: current_user, project: project, author: john_doe, target: issue, action: Todo::MENTIONED, note: note) + create(:todo, user: current_user, project: project, author: john_doe, target: merge_request, action: Todo::ASSIGNED) end - step 'I should see pending tasks assigned to me' do - expect(page).to have_content 'Tasks 4' + step 'I should see todos assigned to me' do + expect(page).to have_content 'Todos 4' expect(page).to have_content 'Done 0' expect(page).to have_link project.name_with_namespace - should_see_task(1, "John Doe assigned merge request ##{merge_request.iid}", merge_request.title) - should_see_task(2, "John Doe mentioned you on issue ##{issue.iid}", "#{current_user.to_reference} Wdyt?") - should_see_task(3, "John Doe assigned issue ##{issue.iid}", issue.title) - should_see_task(4, "Mary Jane mentioned you on issue ##{issue.iid}", issue.title) + should_see_todo(1, "John Doe assigned merge request ##{merge_request.iid}", merge_request.title) + should_see_todo(2, "John Doe mentioned you on issue ##{issue.iid}", "#{current_user.to_reference} Wdyt?") + should_see_todo(3, "John Doe assigned issue ##{issue.iid}", issue.title) + should_see_todo(4, "Mary Jane mentioned you on issue ##{issue.iid}", issue.title) end - step 'I mark the pending task as done' do - page.within('.task:nth-child(1)') do + step 'I mark the todo as done' do + page.within('.todo:nth-child(1)') do click_link 'Done' end - expect(page).to have_content 'Task was successfully marked as done.' - expect(page).to have_content 'Tasks 3' + expect(page).to have_content 'Todo was successfully marked as done.' + expect(page).to have_content 'Todos 3' expect(page).to have_content 'Done 1' - should_not_see_task "John Doe assigned merge request ##{merge_request.iid}" + should_not_see_todo "John Doe assigned merge request ##{merge_request.iid}" end step 'I click on the "Done" tab' do click_link 'Done 1' end - step 'I should see all tasks marked as done' do + step 'I should see all todos marked as done' do expect(page).to have_link project.name_with_namespace - should_see_task(1, "John Doe assigned merge request ##{merge_request.iid}", merge_request.title, false) + should_see_todo(1, "John Doe assigned merge request ##{merge_request.iid}", merge_request.title, false) end step 'I filter by "Enterprise"' do @@ -69,28 +69,28 @@ class Spinach::Features::DashboardTasks < Spinach::FeatureSteps end step 'I filter by "Mentioned"' do - select2("#{Task::MENTIONED}", from: '#action_id') + select2("#{Todo::MENTIONED}", from: '#action_id') end - step 'I should not see tasks' do - expect(page).to have_content 'No tasks to show' + step 'I should not see todos' do + expect(page).to have_content 'No todos to show' end - step 'I should not see tasks related to "Mary Jane" in the list' do - should_not_see_task "Mary Jane mentioned you on issue ##{issue.iid}" + step 'I should not see todos related to "Mary Jane" in the list' do + should_not_see_todo "Mary Jane mentioned you on issue ##{issue.iid}" end - step 'I should not see tasks related to "Merge Requests" in the list' do - should_not_see_task "John Doe assigned merge request ##{merge_request.iid}" + step 'I should not see todos related to "Merge Requests" in the list' do + should_not_see_todo "John Doe assigned merge request ##{merge_request.iid}" end - step 'I should not see tasks related to "Assignments" in the list' do - should_not_see_task "John Doe assigned merge request ##{merge_request.iid}" - should_not_see_task "John Doe assigned issue ##{issue.iid}" + step 'I should not see todos related to "Assignments" in the list' do + should_not_see_todo "John Doe assigned merge request ##{merge_request.iid}" + should_not_see_todo "John Doe assigned issue ##{issue.iid}" end - def should_see_task(position, title, body, pending = true) - page.within(".task:nth-child(#{position})") do + def should_see_todo(position, title, body, pending = true) + page.within(".todo:nth-child(#{position})") do expect(page).to have_content title expect(page).to have_content body @@ -102,7 +102,7 @@ class Spinach::Features::DashboardTasks < Spinach::FeatureSteps end end - def should_not_see_task(title) + def should_not_see_todo(title) expect(page).not_to have_content title end diff --git a/features/steps/shared/paths.rb b/features/steps/shared/paths.rb index 112ace342f0..f4df4874d2f 100644 --- a/features/steps/shared/paths.rb +++ b/features/steps/shared/paths.rb @@ -103,8 +103,8 @@ module SharedPaths visit dashboard_groups_path end - step 'I visit dashboard task queue page' do - visit dashboard_tasks_path + step 'I visit dashboard todos page' do + visit dashboard_todos_path end step 'I should be redirected to the dashboard groups page' do diff --git a/spec/factories/tasks.rb b/spec/factories/todos.rb similarity index 81% rename from spec/factories/tasks.rb rename to spec/factories/todos.rb index 4df489fa4c9..bd85b1d798a 100644 --- a/spec/factories/tasks.rb +++ b/spec/factories/todos.rb @@ -1,6 +1,6 @@ # == Schema Information # -# Table name: tasks +# Table name: todos # # id :integer not null, primary key # user_id :integer not null @@ -16,19 +16,19 @@ # FactoryGirl.define do - factory :task do + factory :todo do project author user target factory: :issue - action { Task::ASSIGNED } + action { Todo::ASSIGNED } trait :assigned do - action { Task::ASSIGNED } + action { Todo::ASSIGNED } end trait :mentioned do - action { Task::MENTIONED } + action { Todo::MENTIONED } end end end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 70c3a999c95..583937ca748 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -27,7 +27,7 @@ describe Note, models: true do it { is_expected.to belong_to(:noteable) } it { is_expected.to belong_to(:author).class_name('User') } - it { is_expected.to have_many(:tasks).dependent(:destroy) } + it { is_expected.to have_many(:todos).dependent(:destroy) } end describe 'validation' do diff --git a/spec/models/task_spec.rb b/spec/models/todo_spec.rb.rb similarity index 94% rename from spec/models/task_spec.rb rename to spec/models/todo_spec.rb.rb index 2bbd47c5e8a..ac481bf9fbd 100644 --- a/spec/models/task_spec.rb +++ b/spec/models/todo_spec.rb.rb @@ -1,6 +1,6 @@ # == Schema Information # -# Table name: tasks +# Table name: todos # # id :integer not null, primary key # user_id :integer not null @@ -17,7 +17,7 @@ require 'spec_helper' -describe Task, models: true do +describe Todo, models: true do describe 'relationships' do it { is_expected.to belong_to(:author).class_name("User") } it { is_expected.to belong_to(:note) } @@ -39,13 +39,13 @@ describe Task, models: true do describe '#action_name' do it 'returns proper message when action is an assigment' do - subject.action = Task::ASSIGNED + subject.action = Todo::ASSIGNED expect(subject.action_name).to eq 'assigned' end it 'returns proper message when action is a mention' do - subject.action = Task::MENTIONED + subject.action = Todo::MENTIONED expect(subject.action_name).to eq 'mentioned you on' end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index d2769836526..32d4f39b04a 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -92,7 +92,7 @@ describe User, models: true do it { is_expected.to have_many(:identities).dependent(:destroy) } it { is_expected.to have_one(:abuse_report) } it { is_expected.to have_many(:spam_logs).dependent(:destroy) } - it { is_expected.to have_many(:tasks).dependent(:destroy) } + it { is_expected.to have_many(:todos).dependent(:destroy) } end describe 'validations' do diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index fdf0fd819b2..62b25709a5d 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -5,7 +5,7 @@ describe Issues::CloseService, services: true do let(:user2) { create(:user) } let(:issue) { create(:issue, assignee: user2) } let(:project) { issue.project } - let!(:pending_task) { create(:task, :assigned, user: user, project: project, target: issue, author: user2) } + let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) } before do project.team << [user, :master] @@ -34,8 +34,8 @@ describe Issues::CloseService, services: true do expect(note.note).to include "Status changed to closed" end - it 'marks pending tasks as done' do - expect(pending_task.reload).to be_done + it 'marks todos as done' do + expect(todo.reload).to be_done end end @@ -47,7 +47,7 @@ describe Issues::CloseService, services: true do it { expect(@issue).to be_valid } it { expect(@issue).to be_opened } - it { expect(pending_task.reload).to be_pending } + it { expect(todo.reload).to be_pending } end end end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index f3b66779987..5e7915db7e1 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -24,17 +24,18 @@ describe Issues::CreateService, services: true do it { expect(@issue.title).to eq('Awesome issue') } it { expect(@issue.assignee).to eq assignee } - it 'creates a pending task for new assignee' do + it 'creates a pending todo for new assignee' do attributes = { project: project, author: user, user: assignee, - target: @issue, - action: Task::ASSIGNED, + target_id: @issue.id, + target_type: @issue.class.name, + action: Todo::ASSIGNED, state: :pending } - expect(Task.where(attributes).count).to eq 1 + expect(Todo.where(attributes).count).to eq 1 end end end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 5674b9f1e9b..e579e49dfa7 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -80,16 +80,16 @@ describe Issues::UpdateService, services: true do end end - context 'task queue' do - let!(:pending_task) { create(:task, :assigned, user: user, project: project, target: issue, author: user2) } + context 'todos' do + let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) } context 'when the title change' do before do update_issue({ title: 'New title' }) end - it 'marks pending tasks as done' do - expect(pending_task.reload.done?).to eq true + it 'marks pending todos as done' do + expect(todo.reload.done?).to eq true end end @@ -98,8 +98,8 @@ describe Issues::UpdateService, services: true do update_issue({ description: 'Also please fix' }) end - it 'marks pending tasks as done' do - expect(pending_task.reload.done?).to eq true + it 'marks todos as done' do + expect(todo.reload.done?).to eq true end end @@ -108,21 +108,22 @@ describe Issues::UpdateService, services: true do update_issue({ assignee: user2 }) end - it 'marks previous assignee pending tasks as done' do - expect(pending_task.reload.done?).to eq true + it 'marks previous assignee todos as done' do + expect(todo.reload.done?).to eq true end - it 'creates a pending task for new assignee' do + it 'creates a todo for new assignee' do attributes = { project: project, author: user, user: user2, - target: issue, - action: Task::ASSIGNED, + target_id: issue.id, + target_type: issue.class.name, + action: Todo::ASSIGNED, state: :pending } - expect(Task.where(attributes).count).to eq 1 + expect(Todo.where(attributes).count).to eq 1 end end @@ -131,8 +132,8 @@ describe Issues::UpdateService, services: true do update_issue({ milestone: create(:milestone) }) end - it 'marks pending tasks as done' do - expect(pending_task.reload.done?).to eq true + it 'marks todos as done' do + expect(todo.reload.done?).to eq true end end @@ -141,8 +142,8 @@ describe Issues::UpdateService, services: true do update_issue({ label_ids: [label.id] }) end - it 'marks pending tasks as done' do - expect(pending_task.reload.done?).to eq true + it 'marks todos as done' do + expect(todo.reload.done?).to eq true end end end diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index 50227af7f2b..8443a00e70c 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -5,7 +5,7 @@ describe MergeRequests::CloseService, services: true do let(:user2) { create(:user) } let(:merge_request) { create(:merge_request, assignee: user2) } let(:project) { merge_request.project } - let!(:pending_task) { create(:task, :assigned, user: user, project: project, target: merge_request, author: user2) } + let!(:todo) { create(:todo, :assigned, user: user, project: project, target: merge_request, author: user2) } before do project.team << [user, :master] @@ -43,8 +43,8 @@ describe MergeRequests::CloseService, services: true do expect(note.note).to include 'Status changed to closed' end - it 'marks pending tasks as done' do - expect(pending_task.reload).to be_done + it 'marks todos as done' do + expect(todo.reload).to be_done end end end diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index 2e3c9f88369..120f4d6a669 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -34,13 +34,14 @@ describe MergeRequests::CreateService, services: true do expect(service).to have_received(:execute_hooks).with(@merge_request) end - it 'does not creates a pending task' do + it 'does not creates todos' do attributes = { project: project, - target: @merge_request + target_id: @merge_request.id, + target_type: @merge_request.class.name } - expect(Task.where(attributes).count).to be_zero + expect(Todo.where(attributes).count).to be_zero end context 'when merge request is assigned to someone' do @@ -56,17 +57,18 @@ describe MergeRequests::CreateService, services: true do it { expect(@merge_request.assignee).to eq assignee } - it 'creates a pending task for new assignee' do + it 'creates a todo for new assignee' do attributes = { project: project, author: user, user: assignee, - target: @merge_request, - action: Task::ASSIGNED, + target_id: @merge_request.id, + target_type: @merge_request.class.name, + action: Todo::ASSIGNED, state: :pending } - expect(Task.where(attributes).count).to eq 1 + expect(Todo.where(attributes).count).to eq 1 end end end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 6d70e8ec16a..99703c7a8ec 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -98,16 +98,16 @@ describe MergeRequests::UpdateService, services: true do end end - context 'task queue' do - let!(:pending_task) { create(:task, :assigned, user: user, project: project, target: merge_request, author: user2) } + context 'todos' do + let!(:pending_todo) { create(:todo, :assigned, user: user, project: project, target: merge_request, author: user2) } context 'when the title change' do before do update_merge_request({ title: 'New title' }) end - it 'marks pending tasks as done' do - expect(pending_task.reload).to be_done + it 'marks pending todos as done' do + expect(pending_todo.reload).to be_done end end @@ -116,8 +116,8 @@ describe MergeRequests::UpdateService, services: true do update_merge_request({ description: 'Also please fix' }) end - it 'marks pending tasks as done' do - expect(pending_task.reload).to be_done + it 'marks pending todos as done' do + expect(pending_todo.reload).to be_done end end @@ -126,21 +126,22 @@ describe MergeRequests::UpdateService, services: true do update_merge_request({ assignee: user2 }) end - it 'marks previous assignee pending tasks as done' do - expect(pending_task.reload).to be_done + it 'marks previous assignee pending todos as done' do + expect(pending_todo.reload).to be_done end - it 'creates a pending task for new assignee' do + it 'creates a pending todo for new assignee' do attributes = { project: project, author: user, user: user2, - target: merge_request, - action: Task::ASSIGNED, + target_id: merge_request.id, + target_type: merge_request.class.name, + action: Todo::ASSIGNED, state: :pending } - expect(Task.where(attributes).count).to eq 1 + expect(Todo.where(attributes).count).to eq 1 end end @@ -149,8 +150,8 @@ describe MergeRequests::UpdateService, services: true do update_merge_request({ milestone: create(:milestone) }) end - it 'marks pending tasks as done' do - expect(pending_task.reload).to be_done + it 'marks pending todos as done' do + expect(pending_todo.reload).to be_done end end @@ -159,8 +160,8 @@ describe MergeRequests::UpdateService, services: true do update_merge_request({ label_ids: [label.id] }) end - it 'marks pending tasks as done' do - expect(pending_task.reload).to be_done + it 'marks pending todos as done' do + expect(pending_todo.reload).to be_done end end @@ -169,8 +170,8 @@ describe MergeRequests::UpdateService, services: true do update_merge_request({ target_branch: 'target' }) end - it 'marks pending tasks as done' do - expect(pending_task.reload).to be_done + it 'marks pending todos as done' do + expect(pending_todo.reload).to be_done end end end diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb index a8b3e0d825d..dde4bde7dc2 100644 --- a/spec/services/notes/update_service_spec.rb +++ b/spec/services/notes/update_service_spec.rb @@ -18,16 +18,16 @@ describe Notes::UpdateService, services: true do @note.reload end - context 'task queue' do - let!(:pending_task) { create(:task, :assigned, user: user, project: project, target: issue, author: user2) } + context 'todos' do + let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) } context 'when the note change' do before do update_note({ note: 'New note' }) end - it 'marks pending tasks as done' do - expect(pending_task.reload).to be_done + it 'marks todos as done' do + expect(todo.reload).to be_done end end @@ -36,8 +36,8 @@ describe Notes::UpdateService, services: true do update_note({ note: 'Old note' }) end - it 'keep pending tasks' do - expect(pending_task.reload).to be_pending + it 'keep todos' do + expect(todo.reload).to be_pending end end end diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb deleted file mode 100644 index 43022ca1604..00000000000 --- a/spec/services/task_service_spec.rb +++ /dev/null @@ -1,264 +0,0 @@ -require 'spec_helper' - -describe TaskService, services: true do - let(:author) { create(:user) } - let(:john_doe) { create(:user, username: 'john_doe') } - let(:michael) { create(:user, username: 'michael') } - let(:stranger) { create(:user, username: 'stranger') } - let(:project) { create(:project) } - let(:mentions) { [author.to_reference, john_doe.to_reference, michael.to_reference, stranger.to_reference].join(' ') } - let(:service) { described_class.new } - - before do - project.team << [author, :developer] - project.team << [john_doe, :developer] - project.team << [michael, :developer] - end - - describe 'Issues' do - let(:issue) { create(:issue, project: project, assignee: john_doe, author: author, description: mentions) } - let(:unassigned_issue) { create(:issue, project: project, assignee: nil) } - - describe '#new_issue' do - it 'creates a task if assigned' do - service.new_issue(issue, author) - - should_create_task(user: john_doe, target: issue, action: Task::ASSIGNED) - end - - it 'does not create a task if unassigned' do - should_not_create_any_task { service.new_issue(unassigned_issue, author) } - end - - it 'does not create a task if assignee is the current user' do - should_not_create_any_task { service.new_issue(unassigned_issue, john_doe) } - end - - it 'creates a task for each valid mentioned user' do - service.new_issue(issue, author) - - should_create_task(user: michael, target: issue, action: Task::MENTIONED) - should_not_create_task(user: author, target: issue, action: Task::MENTIONED) - should_not_create_task(user: john_doe, target: issue, action: Task::MENTIONED) - should_not_create_task(user: stranger, target: issue, action: Task::MENTIONED) - end - end - - describe '#update_issue' do - it 'creates a task for each valid mentioned user' do - service.update_issue(issue, author) - - should_create_task(user: michael, target: issue, action: Task::MENTIONED) - should_create_task(user: john_doe, target: issue, action: Task::MENTIONED) - should_not_create_task(user: author, target: issue, action: Task::MENTIONED) - should_not_create_task(user: stranger, target: issue, action: Task::MENTIONED) - end - - it 'does not create a task if user was already mentioned' do - create(:task, :mentioned, user: michael, project: project, target: issue, author: author) - - expect { service.update_issue(issue, author) }.not_to change(michael.tasks, :count) - end - end - - describe '#close_issue' do - it 'marks related pending tasks to the target for the user as done' do - first_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) - second_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) - - service.close_issue(issue, john_doe) - - expect(first_task.reload).to be_done - expect(second_task.reload).to be_done - end - end - - describe '#reassigned_issue' do - it 'creates a pending task for new assignee' do - unassigned_issue.update_attribute(:assignee, john_doe) - service.reassigned_issue(unassigned_issue, author) - - should_create_task(user: john_doe, target: unassigned_issue, action: Task::ASSIGNED) - end - - it 'does not create a task if unassigned' do - issue.update_attribute(:assignee, nil) - - should_not_create_any_task { service.reassigned_issue(issue, author) } - end - - it 'does not create a task if new assignee is the current user' do - unassigned_issue.update_attribute(:assignee, john_doe) - - should_not_create_any_task { service.reassigned_issue(unassigned_issue, john_doe) } - end - end - - describe '#mark_pending_tasks_as_done' do - it 'marks related pending tasks to the target for the user as done' do - first_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) - second_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) - - service.mark_pending_tasks_as_done(issue, john_doe) - - expect(first_task.reload).to be_done - expect(second_task.reload).to be_done - end - end - - describe '#new_note' do - let!(:first_task) { create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) } - let!(:second_task) { create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) } - let(:note) { create(:note, project: project, noteable: issue, author: john_doe, note: mentions) } - let(:award_note) { create(:note, :award, project: project, noteable: issue, author: john_doe, note: 'thumbsup') } - let(:system_note) { create(:system_note, project: project, noteable: issue) } - - it 'mark related pending tasks to the noteable for the note author as done' do - first_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) - second_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) - - service.new_note(note, john_doe) - - expect(first_task.reload).to be_done - expect(second_task.reload).to be_done - end - - it 'mark related pending tasks to the noteable for the award note author as done' do - service.new_note(award_note, john_doe) - - expect(first_task.reload).to be_done - expect(second_task.reload).to be_done - end - - it 'does not mark related pending tasks it is a system note' do - service.new_note(system_note, john_doe) - - expect(first_task.reload).to be_pending - expect(second_task.reload).to be_pending - end - - it 'creates a task for each valid mentioned user' do - service.new_note(note, john_doe) - - should_create_task(user: michael, target: issue, author: john_doe, action: Task::MENTIONED, note: note) - should_create_task(user: author, target: issue, author: john_doe, action: Task::MENTIONED, note: note) - should_not_create_task(user: john_doe, target: issue, author: john_doe, action: Task::MENTIONED, note: note) - should_not_create_task(user: stranger, target: issue, author: john_doe, action: Task::MENTIONED, note: note) - end - end - end - - describe 'Merge Requests' do - let(:mr_assigned) { create(:merge_request, source_project: project, author: author, assignee: john_doe, description: mentions) } - let(:mr_unassigned) { create(:merge_request, source_project: project, author: author, assignee: nil) } - - describe '#new_merge_request' do - it 'creates a pending task if assigned' do - service.new_merge_request(mr_assigned, author) - - should_create_task(user: john_doe, target: mr_assigned, action: Task::ASSIGNED) - end - - it 'does not create a task if unassigned' do - should_not_create_any_task { service.new_merge_request(mr_unassigned, author) } - end - - it 'does not create a task if assignee is the current user' do - should_not_create_any_task { service.new_merge_request(mr_unassigned, john_doe) } - end - - it 'creates a task for each valid mentioned user' do - service.new_merge_request(mr_assigned, author) - - should_create_task(user: michael, target: mr_assigned, action: Task::MENTIONED) - should_not_create_task(user: author, target: mr_assigned, action: Task::MENTIONED) - should_not_create_task(user: john_doe, target: mr_assigned, action: Task::MENTIONED) - should_not_create_task(user: stranger, target: mr_assigned, action: Task::MENTIONED) - end - end - - describe '#update_merge_request' do - it 'creates a task for each valid mentioned user' do - service.update_merge_request(mr_assigned, author) - - should_create_task(user: michael, target: mr_assigned, action: Task::MENTIONED) - should_create_task(user: john_doe, target: mr_assigned, action: Task::MENTIONED) - should_not_create_task(user: author, target: mr_assigned, action: Task::MENTIONED) - should_not_create_task(user: stranger, target: mr_assigned, action: Task::MENTIONED) - end - - it 'does not create a task if user was already mentioned' do - create(:task, :mentioned, user: michael, project: project, target: mr_assigned, author: author) - - expect { service.update_merge_request(mr_assigned, author) }.not_to change(michael.tasks, :count) - end - end - - describe '#close_merge_request' do - it 'marks related pending tasks to the target for the user as done' do - first_task = create(:task, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) - second_task = create(:task, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) - service.close_merge_request(mr_assigned, john_doe) - - expect(first_task.reload).to be_done - expect(second_task.reload).to be_done - end - end - - describe '#reassigned_merge_request' do - it 'creates a pending task for new assignee' do - mr_unassigned.update_attribute(:assignee, john_doe) - service.reassigned_merge_request(mr_unassigned, author) - - should_create_task(user: john_doe, target: mr_unassigned, action: Task::ASSIGNED) - end - - it 'does not create a task if unassigned' do - mr_assigned.update_attribute(:assignee, nil) - - should_not_create_any_task { service.reassigned_merge_request(mr_assigned, author) } - end - - it 'does not create a task if new assignee is the current user' do - mr_assigned.update_attribute(:assignee, john_doe) - - should_not_create_any_task { service.reassigned_merge_request(mr_assigned, john_doe) } - end - end - - describe '#merge_merge_request' do - it 'marks related pending tasks to the target for the user as done' do - first_task = create(:task, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) - second_task = create(:task, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) - service.merge_merge_request(mr_assigned, john_doe) - - expect(first_task.reload).to be_done - expect(second_task.reload).to be_done - end - end - end - - def should_create_task(attributes = {}) - attributes.reverse_merge!( - project: project, - author: author, - state: :pending - ) - - expect(Task.where(attributes).count).to eq 1 - end - - def should_not_create_task(attributes = {}) - attributes.reverse_merge!( - project: project, - author: author, - state: :pending - ) - - expect(Task.where(attributes).count).to eq 0 - end - - def should_not_create_any_task - expect { yield }.not_to change(Task, :count) - end -end diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb new file mode 100644 index 00000000000..df3aa955f24 --- /dev/null +++ b/spec/services/todo_service_spec.rb @@ -0,0 +1,264 @@ +require 'spec_helper' + +describe TodoService, services: true do + let(:author) { create(:user) } + let(:john_doe) { create(:user, username: 'john_doe') } + let(:michael) { create(:user, username: 'michael') } + let(:stranger) { create(:user, username: 'stranger') } + let(:project) { create(:project) } + let(:mentions) { [author.to_reference, john_doe.to_reference, michael.to_reference, stranger.to_reference].join(' ') } + let(:service) { described_class.new } + + before do + project.team << [author, :developer] + project.team << [john_doe, :developer] + project.team << [michael, :developer] + end + + describe 'Issues' do + let(:issue) { create(:issue, project: project, assignee: john_doe, author: author, description: mentions) } + let(:unassigned_issue) { create(:issue, project: project, assignee: nil) } + + describe '#new_issue' do + it 'creates a todo if assigned' do + service.new_issue(issue, author) + + should_create_todo(user: john_doe, target: issue, action: Todo::ASSIGNED) + end + + it 'does not create a todo if unassigned' do + should_not_create_any_todo { service.new_issue(unassigned_issue, author) } + end + + it 'does not create a todo if assignee is the current user' do + should_not_create_any_todo { service.new_issue(unassigned_issue, john_doe) } + end + + it 'creates a todo for each valid mentioned user' do + service.new_issue(issue, author) + + should_create_todo(user: michael, target: issue, action: Todo::MENTIONED) + should_not_create_todo(user: author, target: issue, action: Todo::MENTIONED) + should_not_create_todo(user: john_doe, target: issue, action: Todo::MENTIONED) + should_not_create_todo(user: stranger, target: issue, action: Todo::MENTIONED) + end + end + + describe '#update_issue' do + it 'creates a todo for each valid mentioned user' do + service.update_issue(issue, author) + + should_create_todo(user: michael, target: issue, action: Todo::MENTIONED) + should_create_todo(user: john_doe, target: issue, action: Todo::MENTIONED) + should_not_create_todo(user: author, target: issue, action: Todo::MENTIONED) + should_not_create_todo(user: stranger, target: issue, action: Todo::MENTIONED) + end + + it 'does not create a todo if user was already mentioned' do + create(:todo, :mentioned, user: michael, project: project, target: issue, author: author) + + expect { service.update_issue(issue, author) }.not_to change(michael.todos, :count) + end + end + + describe '#close_issue' do + it 'marks related pending todos to the target for the user as done' do + first_todo = create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) + second_todo = create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) + + service.close_issue(issue, john_doe) + + expect(first_todo.reload).to be_done + expect(second_todo.reload).to be_done + end + end + + describe '#reassigned_issue' do + it 'creates a pending todo for new assignee' do + unassigned_issue.update_attribute(:assignee, john_doe) + service.reassigned_issue(unassigned_issue, author) + + should_create_todo(user: john_doe, target: unassigned_issue, action: Todo::ASSIGNED) + end + + it 'does not create a todo if unassigned' do + issue.update_attribute(:assignee, nil) + + should_not_create_any_todo { service.reassigned_issue(issue, author) } + end + + it 'does not create a todo if new assignee is the current user' do + unassigned_issue.update_attribute(:assignee, john_doe) + + should_not_create_any_todo { service.reassigned_issue(unassigned_issue, john_doe) } + end + end + + describe '#mark_pending_todos_as_done' do + it 'marks related pending todos to the target for the user as done' do + first_todo = create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) + second_todo = create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) + + service.mark_pending_todos_as_done(issue, john_doe) + + expect(first_todo.reload).to be_done + expect(second_todo.reload).to be_done + end + end + + describe '#new_note' do + let!(:first_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) } + let!(:second_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) } + let(:note) { create(:note, project: project, noteable: issue, author: john_doe, note: mentions) } + let(:award_note) { create(:note, :award, project: project, noteable: issue, author: john_doe, note: 'thumbsup') } + let(:system_note) { create(:system_note, project: project, noteable: issue) } + + it 'mark related pending todos to the noteable for the note author as done' do + first_todo = create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) + second_todo = create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) + + service.new_note(note, john_doe) + + expect(first_todo.reload).to be_done + expect(second_todo.reload).to be_done + end + + it 'mark related pending todos to the noteable for the award note author as done' do + service.new_note(award_note, john_doe) + + expect(first_todo.reload).to be_done + expect(second_todo.reload).to be_done + end + + it 'does not mark related pending todos it is a system note' do + service.new_note(system_note, john_doe) + + expect(first_todo.reload).to be_pending + expect(second_todo.reload).to be_pending + end + + it 'creates a todo for each valid mentioned user' do + service.new_note(note, john_doe) + + should_create_todo(user: michael, target: issue, author: john_doe, action: Todo::MENTIONED, note: note) + should_create_todo(user: author, target: issue, author: john_doe, action: Todo::MENTIONED, note: note) + should_not_create_todo(user: john_doe, target: issue, author: john_doe, action: Todo::MENTIONED, note: note) + should_not_create_todo(user: stranger, target: issue, author: john_doe, action: Todo::MENTIONED, note: note) + end + end + end + + describe 'Merge Requests' do + let(:mr_assigned) { create(:merge_request, source_project: project, author: author, assignee: john_doe, description: mentions) } + let(:mr_unassigned) { create(:merge_request, source_project: project, author: author, assignee: nil) } + + describe '#new_merge_request' do + it 'creates a pending todo if assigned' do + service.new_merge_request(mr_assigned, author) + + should_create_todo(user: john_doe, target: mr_assigned, action: Todo::ASSIGNED) + end + + it 'does not create a todo if unassigned' do + should_not_create_any_todo { service.new_merge_request(mr_unassigned, author) } + end + + it 'does not create a todo if assignee is the current user' do + should_not_create_any_todo { service.new_merge_request(mr_unassigned, john_doe) } + end + + it 'creates a todo for each valid mentioned user' do + service.new_merge_request(mr_assigned, author) + + should_create_todo(user: michael, target: mr_assigned, action: Todo::MENTIONED) + should_not_create_todo(user: author, target: mr_assigned, action: Todo::MENTIONED) + should_not_create_todo(user: john_doe, target: mr_assigned, action: Todo::MENTIONED) + should_not_create_todo(user: stranger, target: mr_assigned, action: Todo::MENTIONED) + end + end + + describe '#update_merge_request' do + it 'creates a todo for each valid mentioned user' do + service.update_merge_request(mr_assigned, author) + + should_create_todo(user: michael, target: mr_assigned, action: Todo::MENTIONED) + should_create_todo(user: john_doe, target: mr_assigned, action: Todo::MENTIONED) + should_not_create_todo(user: author, target: mr_assigned, action: Todo::MENTIONED) + should_not_create_todo(user: stranger, target: mr_assigned, action: Todo::MENTIONED) + end + + it 'does not create a todo if user was already mentioned' do + create(:todo, :mentioned, user: michael, project: project, target: mr_assigned, author: author) + + expect { service.update_merge_request(mr_assigned, author) }.not_to change(michael.todos, :count) + end + end + + describe '#close_merge_request' do + it 'marks related pending todos to the target for the user as done' do + first_todo = create(:todo, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) + second_todo = create(:todo, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) + service.close_merge_request(mr_assigned, john_doe) + + expect(first_todo.reload).to be_done + expect(second_todo.reload).to be_done + end + end + + describe '#reassigned_merge_request' do + it 'creates a pending todo for new assignee' do + mr_unassigned.update_attribute(:assignee, john_doe) + service.reassigned_merge_request(mr_unassigned, author) + + should_create_todo(user: john_doe, target: mr_unassigned, action: Todo::ASSIGNED) + end + + it 'does not create a todo if unassigned' do + mr_assigned.update_attribute(:assignee, nil) + + should_not_create_any_todo { service.reassigned_merge_request(mr_assigned, author) } + end + + it 'does not create a todo if new assignee is the current user' do + mr_assigned.update_attribute(:assignee, john_doe) + + should_not_create_any_todo { service.reassigned_merge_request(mr_assigned, john_doe) } + end + end + + describe '#merge_merge_request' do + it 'marks related pending todos to the target for the user as done' do + first_todo = create(:todo, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) + second_todo = create(:todo, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) + service.merge_merge_request(mr_assigned, john_doe) + + expect(first_todo.reload).to be_done + expect(second_todo.reload).to be_done + end + end + end + + def should_create_todo(attributes = {}) + attributes.reverse_merge!( + project: project, + author: author, + state: :pending + ) + + expect(Todo.where(attributes).count).to eq 1 + end + + def should_not_create_todo(attributes = {}) + attributes.reverse_merge!( + project: project, + author: author, + state: :pending + ) + + expect(Todo.where(attributes).count).to eq 0 + end + + def should_not_create_any_todo + expect { yield }.not_to change(Todo, :count) + end +end -- 2.18.1 From f42b1fa6e5d84b84ede5bcc853dd2df9c6b5836d Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Sat, 20 Feb 2016 13:10:37 -0200 Subject: [PATCH 43/50] Fix rubucop offenses --- app/controllers/dashboard/todos_controller.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb index e24b8d3e58f..ed11ca5c0eb 100644 --- a/app/controllers/dashboard/todos_controller.rb +++ b/app/controllers/dashboard/todos_controller.rb @@ -19,5 +19,3 @@ class Dashboard::TodosController < Dashboard::ApplicationController @todo ||= current_user.todos.find(params[:id]) end end - - -- 2.18.1 From 0e9775816a13feabf1ae0d4940ef30bba1196d40 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sat, 20 Feb 2016 11:52:57 -0800 Subject: [PATCH 44/50] Rename tab from 'Todos' to 'To do' --- app/views/dashboard/todos/index.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/dashboard/todos/index.html.haml b/app/views/dashboard/todos/index.html.haml index a91f8f24f8c..8340c799c53 100644 --- a/app/views/dashboard/todos/index.html.haml +++ b/app/views/dashboard/todos/index.html.haml @@ -6,7 +6,7 @@ %li{class: ('active' if params[:state].blank? || params[:state] == 'pending')} = link_to page_filter_path(state: 'pending') do %span - Todos + To do %span{class: 'badge'} = todos_pending_count %li{class: ('active' if params[:state] == 'done')} -- 2.18.1 From 57d16552ff769cc5b41737de803bc2ddc4813f4e Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sat, 20 Feb 2016 11:53:15 -0800 Subject: [PATCH 45/50] Include 'issue'/'merge request' in link --- app/helpers/todos_helper.rb | 11 +++-------- app/views/dashboard/todos/_todo.html.haml | 4 ++-- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb index 7c360468945..cbbfc1e04f5 100644 --- a/app/helpers/todos_helper.rb +++ b/app/helpers/todos_helper.rb @@ -7,14 +7,9 @@ module TodosHelper current_user.todos.done.count end - def todo_action_name(todo) - target = todo.target_type.titleize.downcase - - [todo.action_name, target].join(" ") - end - - def todo_target_link_html(todo) - link_to "##{todo.target_iid}", todo_target_path(todo) + def todo_target_link(todo) + target = todo.target_type.titleize.downcase + link_to "#{target} #{todo.target.to_reference}", todo_target_path(todo) end def todo_target_path(todo) diff --git a/app/views/dashboard/todos/_todo.html.haml b/app/views/dashboard/todos/_todo.html.haml index 7de703e304f..773a9f6f4b5 100644 --- a/app/views/dashboard/todos/_todo.html.haml +++ b/app/views/dashboard/todos/_todo.html.haml @@ -6,8 +6,8 @@ %span.author_name = link_to_author todo %span.todo_label - = todo_action_name(todo) - = todo_target_link_html(todo) + = todo.action_name + = todo_target_link(todo) · #{time_ago_with_tooltip(todo.created_at)} -- 2.18.1 From d53ae7f14d7c24ee590ba57632d267ec01706aa4 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sat, 20 Feb 2016 11:53:25 -0800 Subject: [PATCH 46/50] Add "Mark all as done" button --- app/controllers/dashboard/todos_controller.rb | 16 +++++++++++- app/helpers/todos_helper.rb | 26 +++++++++++++++++++ app/views/dashboard/todos/index.html.haml | 12 ++++++--- 3 files changed, 49 insertions(+), 5 deletions(-) diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb index ed11ca5c0eb..4e8ebfb06a0 100644 --- a/app/controllers/dashboard/todos_controller.rb +++ b/app/controllers/dashboard/todos_controller.rb @@ -1,6 +1,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController + before_filter :find_todos, only: [:index, :destroy_all] + def index - @todos = TodosFinder.new(current_user, params).execute @todos = @todos.page(params[:page]).per(PER_PAGE) end @@ -13,9 +14,22 @@ class Dashboard::TodosController < Dashboard::ApplicationController end end + def destroy_all + @todos.each(&:done) + + respond_to do |format| + format.html { redirect_to dashboard_todos_path, notice: 'All todos were marked as done.' } + format.js { render nothing: true } + end + end + private def todo @todo ||= current_user.todos.find(params[:id]) end + + def find_todos + @todos = TodosFinder.new(current_user, params).execute + end end diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb index cbbfc1e04f5..9bcfbd2da35 100644 --- a/app/helpers/todos_helper.rb +++ b/app/helpers/todos_helper.rb @@ -19,6 +19,32 @@ module TodosHelper todo.project, todo.target], anchor: anchor) end + def todos_filter_params + { + state: params[:state], + project_id: params[:project_id], + author_id: params[:author_id], + type: params[:type], + action_id: params[:action_id], + } + end + + def todos_filter_path(options = {}) + without = options.delete(:without) + + options = todos_filter_params.merge(options) + + if without.present? + without.each do |key| + options.delete(key) + end + end + + path = request.path + path << "?#{options.to_param}" + path + end + def todo_actions_options actions = [ OpenStruct.new(id: '', title: 'Any Action'), diff --git a/app/views/dashboard/todos/index.html.haml b/app/views/dashboard/todos/index.html.haml index 8340c799c53..946d7df3933 100644 --- a/app/views/dashboard/todos/index.html.haml +++ b/app/views/dashboard/todos/index.html.haml @@ -4,21 +4,25 @@ .top-area %ul.nav-links %li{class: ('active' if params[:state].blank? || params[:state] == 'pending')} - = link_to page_filter_path(state: 'pending') do + = link_to todos_filter_path(state: 'pending') do %span To do %span{class: 'badge'} = todos_pending_count %li{class: ('active' if params[:state] == 'done')} - = link_to page_filter_path(state: 'done') do + = link_to todos_filter_path(state: 'done') do %span Done %span{class: 'badge'} = todos_done_count + .nav-controls + - if @todos.any?(&:pending?) + = link_to 'Mark all as done', destroy_all_dashboard_todos_path(todos_filter_params), class: 'btn', method: :delete + .todos-filters .gray-content-block.second-block - = form_tag page_filter_path(without: [:assignee_id, :milestone_title, :label_name, :scope, :sort]), method: :get, class: 'filter-form' do + = form_tag todos_filter_path(without: [:project_id, :author_id, :type, :action_id]), method: :get, class: 'filter-form' do .filter-item.inline = select_tag('project_id', todo_projects_options, class: 'select2 trigger-submit', include_blank: true, @@ -47,7 +51,7 @@ = render group[1] = paginate @todos, theme: "gitlab" - else - .nothing-here-block No todos to show + .nothing-here-block You're all done! :javascript new UsersSelect(); -- 2.18.1 From 42cbcb23478f7b07c0c49e0020a8d3c834322e87 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sat, 20 Feb 2016 12:01:50 -0800 Subject: [PATCH 47/50] "X assigned you Y" instead of "X assigned Y" --- app/helpers/todos_helper.rb | 7 +++++++ app/models/todo.rb | 11 ----------- app/views/dashboard/todos/_todo.html.haml | 2 +- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb index 9bcfbd2da35..4b745a5b969 100644 --- a/app/helpers/todos_helper.rb +++ b/app/helpers/todos_helper.rb @@ -7,6 +7,13 @@ module TodosHelper current_user.todos.done.count end + def todo_action_name(todo) + case todo.action + when Todo::ASSIGNED then 'assigned you' + when Todo::MENTIONED then 'mentioned you on' + end + end + def todo_target_link(todo) target = todo.target_type.titleize.downcase link_to "#{target} #{todo.target.to_reference}", todo_target_path(todo) diff --git a/app/models/todo.rb b/app/models/todo.rb index 7a9b0212f9f..34d71c1b0d3 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -43,13 +43,6 @@ class Todo < ActiveRecord::Base state :done end - def action_name - case action - when ASSIGNED then 'assigned' - when MENTIONED then 'mentioned you on' - end - end - def body if note.present? note.note @@ -57,8 +50,4 @@ class Todo < ActiveRecord::Base target.title end end - - def target_iid - target.respond_to?(:iid) ? target.iid : target_id - end end diff --git a/app/views/dashboard/todos/_todo.html.haml b/app/views/dashboard/todos/_todo.html.haml index 773a9f6f4b5..6975f6ed0db 100644 --- a/app/views/dashboard/todos/_todo.html.haml +++ b/app/views/dashboard/todos/_todo.html.haml @@ -6,7 +6,7 @@ %span.author_name = link_to_author todo %span.todo_label - = todo.action_name + = todo_action_name(todo) = todo_target_link(todo) · #{time_ago_with_tooltip(todo.created_at)} -- 2.18.1 From 7f3026854515d0c9d713ad9356016a1fa55a3200 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sat, 20 Feb 2016 12:05:57 -0800 Subject: [PATCH 48/50] Update feature spec --- features/steps/dashboard/todos.rb | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/features/steps/dashboard/todos.rb b/features/steps/dashboard/todos.rb index 207d997f8a0..9722a5a848c 100644 --- a/features/steps/dashboard/todos.rb +++ b/features/steps/dashboard/todos.rb @@ -26,13 +26,13 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps end step 'I should see todos assigned to me' do - expect(page).to have_content 'Todos 4' + expect(page).to have_content 'To do 4' expect(page).to have_content 'Done 0' expect(page).to have_link project.name_with_namespace - should_see_todo(1, "John Doe assigned merge request ##{merge_request.iid}", merge_request.title) + should_see_todo(1, "John Doe assigned you merge request !#{merge_request.iid}", merge_request.title) should_see_todo(2, "John Doe mentioned you on issue ##{issue.iid}", "#{current_user.to_reference} Wdyt?") - should_see_todo(3, "John Doe assigned issue ##{issue.iid}", issue.title) + should_see_todo(3, "John Doe assigned you issue ##{issue.iid}", issue.title) should_see_todo(4, "Mary Jane mentioned you on issue ##{issue.iid}", issue.title) end @@ -42,9 +42,9 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps end expect(page).to have_content 'Todo was successfully marked as done.' - expect(page).to have_content 'Todos 3' + expect(page).to have_content 'To do 3' expect(page).to have_content 'Done 1' - should_not_see_todo "John Doe assigned merge request ##{merge_request.iid}" + should_not_see_todo "John Doe assigned you merge request !#{merge_request.iid}" end step 'I click on the "Done" tab' do @@ -53,7 +53,7 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps step 'I should see all todos marked as done' do expect(page).to have_link project.name_with_namespace - should_see_todo(1, "John Doe assigned merge request ##{merge_request.iid}", merge_request.title, false) + should_see_todo(1, "John Doe assigned you merge request !#{merge_request.iid}", merge_request.title, false) end step 'I filter by "Enterprise"' do @@ -73,7 +73,7 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps end step 'I should not see todos' do - expect(page).to have_content 'No todos to show' + expect(page).to have_content "You're all done!" end step 'I should not see todos related to "Mary Jane" in the list' do @@ -81,12 +81,12 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps end step 'I should not see todos related to "Merge Requests" in the list' do - should_not_see_todo "John Doe assigned merge request ##{merge_request.iid}" + should_not_see_todo "John Doe assigned you merge request !#{merge_request.iid}" end step 'I should not see todos related to "Assignments" in the list' do - should_not_see_todo "John Doe assigned merge request ##{merge_request.iid}" - should_not_see_todo "John Doe assigned issue ##{issue.iid}" + should_not_see_todo "John Doe assigned you merge request !#{merge_request.iid}" + should_not_see_todo "John Doe assigned you issue ##{issue.iid}" end def should_see_todo(position, title, body, pending = true) -- 2.18.1 From f37765efa237fdc26b9937bcb2128910eccbaf91 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Sun, 21 Feb 2016 12:52:37 -0300 Subject: [PATCH 49/50] Use before_action instead of before_filter --- app/controllers/dashboard/todos_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb index 4e8ebfb06a0..9ee9039f004 100644 --- a/app/controllers/dashboard/todos_controller.rb +++ b/app/controllers/dashboard/todos_controller.rb @@ -1,5 +1,5 @@ class Dashboard::TodosController < Dashboard::ApplicationController - before_filter :find_todos, only: [:index, :destroy_all] + before_action :find_todos, only: [:index, :destroy_all] def index @todos = @todos.page(params[:page]).per(PER_PAGE) -- 2.18.1 From 2ef1ea501f48e29399d94ec5843984e4f7014b99 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Sun, 21 Feb 2016 12:57:12 -0300 Subject: [PATCH 50/50] Add missing route to mark all todos as done --- config/routes.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/config/routes.rb b/config/routes.rb index c228a51bed1..30681356c5f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -333,7 +333,12 @@ Rails.application.routes.draw do resources :groups, only: [:index] resources :snippets, only: [:index] - resources :todos, only: [:index, :destroy] + + resources :todos, only: [:index, :destroy] do + collection do + delete :destroy_all + end + end resources :projects, only: [:index] do collection do -- 2.18.1