From 41f1b3deded73eb30be47503ca123877debcf5e6 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 22 Feb 2016 02:36:41 +0000 Subject: Merge branch 'tasks' into 'master' Add Todos Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/2425 Tasks: - Prepare database - [X] Create a new table (`todos`) - Tasks Queue view - [X] Add a number icon showing the number of todos on the top right next to the new and logout button that will redirect the user to the todos page - [X] Add a chronological list of todos, with the 'Todos' tab active by default - [X] Add a 'Done' button to each todo - [x] Add filters (project, author, type, and action) - Todos generation - [X] When user issue/mr is assgined to someone - [x] When user is mentioned on (issues/mr's/comments) - Mark todo as `done` - [X] When clicks on the 'Done' button - [X] When edit issue/mr - [X] When left/edit a comment - [X] When reassign issue/mr - [X] When add/remove labels to issue/mr - [X] When issue/mr is closed - [X] When mr is merged - [X] When added an emoji - [X] When changed the issue/mr milestone * Screenshot: ![Screenshot_2016-02-20_12.45.57](/uploads/4b2554b1bde25aed3347e1ae41e8e0c0/Screenshot_2016-02-20_12.45.57.png) See merge request !2817 --- CHANGELOG | 3 +- app/assets/stylesheets/pages/todos.scss | 124 ++++++++++ app/controllers/dashboard/todos_controller.rb | 35 +++ .../projects/merge_requests_controller.rb | 2 + app/finders/todos_finder.rb | 129 ++++++++++ app/helpers/todos_helper.rb | 87 +++++++ app/models/note.rb | 2 + app/models/todo.rb | 53 +++++ app/models/user.rb | 2 +- app/services/base_service.rb | 4 + app/services/issuable_base_service.rb | 15 +- app/services/issues/close_service.rb | 2 + app/services/issues/create_service.rb | 1 + app/services/issues/update_service.rb | 12 +- app/services/merge_requests/close_service.rb | 1 + app/services/merge_requests/create_service.rb | 3 +- app/services/merge_requests/update_service.rb | 12 +- app/services/notes/create_service.rb | 1 + app/services/notes/post_process_service.rb | 2 - app/services/notes/update_service.rb | 4 + app/services/todo_service.rb | 170 +++++++++++++ app/views/dashboard/todos/_todo.html.haml | 21 ++ app/views/dashboard/todos/index.html.haml | 62 +++++ app/views/layouts/header/_default.html.haml | 6 +- app/views/layouts/nav/_dashboard.html.haml | 18 +- config/routes.rb | 6 + db/migrate/20160212123307_create_tasks.rb | 14 ++ db/migrate/20160217174422_add_note_to_tasks.rb | 5 + db/migrate/20160220123949_rename_tasks_to_todos.rb | 5 + db/schema.rb | 22 +- features/dashboard/todos.feature | 38 +++ features/steps/dashboard/todos.rb | 128 ++++++++++ features/steps/shared/paths.rb | 4 + spec/factories/todos.rb | 34 +++ spec/models/note_spec.rb | 2 + spec/models/todo_spec.rb.rb | 89 +++++++ spec/models/user_spec.rb | 1 + spec/services/issues/close_service_spec.rb | 6 + spec/services/issues/create_service_spec.rb | 23 +- spec/services/issues/update_service_spec.rb | 69 +++++- spec/services/merge_requests/close_service_spec.rb | 5 + .../services/merge_requests/create_service_spec.rb | 42 ++++ .../services/merge_requests/update_service_spec.rb | 79 +++++- spec/services/notes/post_process_service_spec.rb | 1 + spec/services/notes/update_service_spec.rb | 45 ++++ spec/services/todo_service_spec.rb | 264 +++++++++++++++++++++ 46 files changed, 1633 insertions(+), 20 deletions(-) create mode 100644 app/assets/stylesheets/pages/todos.scss create mode 100644 app/controllers/dashboard/todos_controller.rb create mode 100644 app/finders/todos_finder.rb create mode 100644 app/helpers/todos_helper.rb create mode 100644 app/models/todo.rb create mode 100644 app/services/todo_service.rb create mode 100644 app/views/dashboard/todos/_todo.html.haml create mode 100644 app/views/dashboard/todos/index.html.haml create mode 100644 db/migrate/20160212123307_create_tasks.rb create mode 100644 db/migrate/20160217174422_add_note_to_tasks.rb create mode 100644 db/migrate/20160220123949_rename_tasks_to_todos.rb create mode 100644 features/dashboard/todos.feature create mode 100644 features/steps/dashboard/todos.rb create mode 100644 spec/factories/todos.rb create mode 100644 spec/models/todo_spec.rb.rb create mode 100644 spec/services/notes/update_service_spec.rb create mode 100644 spec/services/todo_service_spec.rb diff --git a/CHANGELOG b/CHANGELOG index b31e134918c..77a21e757c4 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -67,7 +67,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. @@ -77,6 +77,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 Todos v 8.4.4 - Update omniauth-saml gem to 1.4.2 diff --git a/app/assets/stylesheets/pages/todos.scss b/app/assets/stylesheets/pages/todos.scss new file mode 100644 index 00000000000..2f57f21963d --- /dev/null +++ b/app/assets/stylesheets/pages/todos.scss @@ -0,0 +1,124 @@ +/** + * Dashboard Todos + * + */ + +.navbar-nav { + li { + .badge.todos-pending-count { + background-color: #7f8fa4; + margin-top: -5px; + } + } +} + +.todos { + .panel { + border-top: none; + margin-bottom: 0; + } +} + +.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; + + &.todo-inline { + .avatar { + position: relative; + top: -2px; + } + + .todo-title { + line-height: 40px; + } + } + + a { + color: #4c4e54; + } + + .avatar { + margin-left: -($gl-avatar-size + $gl-padding-top); + } + + .todo-title { + @include str-truncated(calc(100% - 174px)); + font-weight: 600; + + .author_name { + color: #333; + } + } + + .todo-body { + margin-right: 174px; + + .todo-note { + word-wrap: break-word; + + .md { + color: #7f8fa4; + font-size: $gl-font-size; + + p { + color: #5c5d5e; + } + } + + pre { + border: none; + background: #f9f9f9; + border-radius: 0; + color: #777; + margin: 0 20px; + overflow: hidden; + } + + .note-image-attach { + margin-top: 4px; + margin-left: 0px; + max-width: 200px; + float: none; + } + + p:last-child { + margin-bottom: 0; + } + } + + .todo-note-icon { + color: #777; + float: left; + font-size: $gl-font-size; + line-height: 16px; + margin-right: 5px; + } + } + + &:last-child { border:none } +} + +@media (max-width: $screen-xs-max) { + .todo-item { + padding-left: $gl-padding; + + .todo-title { + white-space: normal; + overflow: visible; + max-width: 100%; + } + + .avatar { + display: none; + } + + .todo-body { + margin: 0; + border-left: 2px solid #DDD; + padding-left: 10px; + } + } +} diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb new file mode 100644 index 00000000000..9ee9039f004 --- /dev/null +++ b/app/controllers/dashboard/todos_controller.rb @@ -0,0 +1,35 @@ +class Dashboard::TodosController < Dashboard::ApplicationController + before_action :find_todos, only: [:index, :destroy_all] + + def index + @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 + + 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/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 86b8e7bdf2e..5fe21694605 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 + TodoService.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/finders/todos_finder.rb b/app/finders/todos_finder.rb new file mode 100644 index 00000000000..3ba27c40504 --- /dev/null +++ b/app/finders/todos_finder.rb @@ -0,0 +1,129 @@ +# TodosFinder +# +# Used to filter Todos 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 TodosFinder + NONE = '0' + + attr_accessor :current_user, :params + + def initialize(current_user, params) + @current_user = current_user + @params = params + end + + def execute + items = current_user.todos + 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? && [Todo::ASSIGNED, Todo::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/todos_helper.rb b/app/helpers/todos_helper.rb new file mode 100644 index 00000000000..4b745a5b969 --- /dev/null +++ b/app/helpers/todos_helper.rb @@ -0,0 +1,87 @@ +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) + 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) + 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 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'), + 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 b3809ad81e0..d287e0f3c6d 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 :todos, dependent: :destroy + delegate :name, to: :project, prefix: true delegate :name, :email, to: :author, prefix: true diff --git a/app/models/todo.rb b/app/models/todo.rb new file mode 100644 index 00000000000..34d71c1b0d3 --- /dev/null +++ b/app/models/todo.rb @@ -0,0 +1,53 @@ +# == Schema Information +# +# Table name: todos +# +# 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 +# note_id :integer +# action :integer not null +# state :string not null +# created_at :datetime +# updated_at :datetime +# + +class Todo < ActiveRecord::Base + ASSIGNED = 1 + MENTIONED = 2 + + belongs_to :author, class_name: "User" + belongs_to :note + 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 + event :done do + transition pending: :done + end + + state :pending + state :done + end + + def body + if note.present? + note.note + else + target.title + end + end +end diff --git a/app/models/user.rb b/app/models/user.rb index 9fe94b13e52..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 :todos, dependent: :destroy # # Validations diff --git a/app/services/base_service.rb b/app/services/base_service.rb index b48ca67d4d2..8563633816c 100644 --- a/app/services/base_service.rb +++ b/app/services/base_service.rb @@ -23,6 +23,10 @@ class BaseService EventCreateService.new end + def todo_service + TodoService.new + end + def log_info(message) Gitlab::AppLogger.info message end diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 2556f06e2d3..ca87dca4a70 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 @@ -71,6 +71,19 @@ class IssuableBaseService < BaseService end end + def has_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/close_service.rb b/app/services/issues/close_service.rb index a1a20e47681..78254b49af3 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) + todo_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) + 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 bcb380d3215..10787e8873c 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) + 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 a55a04dd5e0..51ef9dfe610 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -4,7 +4,16 @@ module Issues update(issue) end - def handle_changes(issue) + def handle_changes(issue, options = {}) + if has_changes?(issue, options) + todo_service.mark_pending_todos_as_done(issue, current_user) + end + + if issue.previous_changes.include?('title') || + issue.previous_changes.include?('description') + todo_service.update_issue(issue, current_user) + end + if issue.previous_changes.include?('milestone_id') create_milestone_note(issue) end @@ -12,6 +21,7 @@ module Issues if issue.previous_changes.include?('assignee_id') create_assignee_note(issue) notification_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 47454f9f0c2..27ee81fe3e7 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) + 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 009d5a6867e..33609d01f20 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) + 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 5ff2cc03dda..6319ad805b6 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -14,7 +14,16 @@ module MergeRequests update(merge_request) end - def handle_changes(merge_request) + def handle_changes(merge_request, options = {}) + if has_changes?(merge_request, options) + 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') + todo_service.update_merge_request(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, @@ -28,6 +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) + 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 8d9661167b5..b970439b921 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) + TodoService.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 f37d3c50cdd..e818f58d13c 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) @@ -25,6 +24,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/notes/update_service.rb b/app/services/notes/update_service.rb index 72e2f78008d..1361b1e0300 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') + TodoService.new.update_note(note, current_user) + end + note end end diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb new file mode 100644 index 00000000000..dc270602ebc --- /dev/null +++ b/app/services/todo_service.rb @@ -0,0 +1,170 @@ +# TodoService class +# +# Used for creating todos after certain user actions +# +# Ex. +# TodoService.new.new_issue(issue, current_user) +# +class TodoService + # When create an issue we should: + # + # * 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) + end + + # When update an issue we should: + # + # * mark all pending todos related to the issue for the current user as done + # + def update_issue(issue, current_user) + create_mention_todos(issue.project, issue, current_user) + end + + # When close an issue we should: + # + # * mark all pending todos related to the target for the current user as done + # + def close_issue(issue, current_user) + mark_pending_todos_as_done(issue, current_user) + end + + # When we reassign an issue we should: + # + # * create a pending todo for new assignee if issue is assigned + # + def reassigned_issue(issue, current_user) + create_assignment_todo(issue, current_user) + end + + # When create a merge request we should: + # + # * 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) + end + + # When update a merge request we should: + # + # * create a todo for each mentioned user on merge request + # + def update_merge_request(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 todos related to the target for the current user as done + # + def close_merge_request(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 todo for new assignee if merge request is assigned + # + def reassigned_merge_request(merge_request, current_user) + create_assignment_todo(merge_request, current_user) + end + + # When merge a merge request we should: + # + # * mark all pending todos related to the target for the current user as done + # + def merge_merge_request(merge_request, current_user) + mark_pending_todos_as_done(merge_request, current_user) + end + + # When create a note we should: + # + # * 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) + end + + # When update a note we should: + # + # * 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 todos as done we should: + # + # * mark all pending todos related to the target for the current user as done + # + def mark_pending_todos_as_done(target, user) + pending_todos(user, target.project, target).update_all(state: :done) + end + + private + + def create_todos(project, target, author, users, action, note = nil) + Array(users).each do |user| + next if pending_todos(user, project, target).exists? + + Todo.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_todo(issuable, author) + create_mention_todos(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 + + mark_pending_todos_as_done(target, author) + create_mention_todos(project, target, author, note) + end + + def create_assignment_todo(issuable, author) + if issuable.assignee && issuable.assignee != author + create_todos(issuable.project, issuable, author, issuable.assignee, Todo::ASSIGNED) + end + end + + def create_mention_todos(project, issuable, author, note = nil) + mentioned_users = filter_mentioned_users(project, note || issuable, author) + create_todos(project, issuable, author, mentioned_users, Todo::MENTIONED, note) + end + + def filter_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.uniq + end + + def pending_todos(user, project, target) + user.todos.pending.where( + project_id: project.id, + target_id: target.id, + target_type: target.class.name + ) + end +end diff --git a/app/views/dashboard/todos/_todo.html.haml b/app/views/dashboard/todos/_todo.html.haml new file mode 100644 index 00000000000..6975f6ed0db --- /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(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/todos/index.html.haml b/app/views/dashboard/todos/index.html.haml new file mode 100644 index 00000000000..946d7df3933 --- /dev/null +++ b/app/views/dashboard/todos/index.html.haml @@ -0,0 +1,62 @@ +- 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 todos_filter_path(state: 'pending') do + %span + To do + %span{class: 'badge'} + = todos_pending_count + %li{class: ('active' if params[:state] == 'done')} + = 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 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, + 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', todo_types_options, + class: 'select2 trigger-submit', include_blank: true, + data: {placeholder: 'Type'}) + .filter-item.inline.actions-filter + = select_tag('action_id', todo_actions_options, + class: 'select2 trigger-submit', include_blank: true, + data: {placeholder: 'Action'}) + +.prepend-top-default + - 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.todos-list + = render group[1] + = paginate @todos, theme: "gitlab" + - else + .nothing-here-block You're all done! + +:javascript + new UsersSelect(); + + $('form.filter-form').on('submit', function (event) { + event.preventDefault(); + Turbolinks.visit(this.action + '&' + $(this).serialize()); + }); diff --git a/app/views/layouts/header/_default.html.haml b/app/views/layouts/header/_default.html.haml index fcb6b835a7e..4781ff23507 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_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 @@ -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)}"; diff --git a/app/views/layouts/nav/_dashboard.html.haml b/app/views/layouts/nav/_dashboard.html.haml index 106abd24a56..db0cf393922 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: :todos) do + = link_to dashboard_todos_path, title: 'Todos' do + = icon('bell fw') + %span + 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') @@ -25,12 +31,12 @@ %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(controller: :snippets) do = link_to dashboard_snippets_path, title: 'Snippets' do = icon('clipboard fw') diff --git a/config/routes.rb b/config/routes.rb index 0ed3f1731f8..30681356c5f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -334,6 +334,12 @@ Rails.application.routes.draw do resources :groups, only: [:index] resources :snippets, only: [:index] + resources :todos, only: [:index, :destroy] do + collection do + delete :destroy_all + end + end + resources :projects, only: [:index] do collection do get :starred 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/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/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 af5bac63b42..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: 20160217100506) do +ActiveRecord::Schema.define(version: 20160220123949) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -824,6 +824,26 @@ ActiveRecord::Schema.define(version: 20160217100506) do add_index "tags", ["name"], name: "index_tags_on_name", unique: true, using: :btree + 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 + 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" + t.integer "note_id" + end + + 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 t.string "encrypted_password", default: "", null: false diff --git a/features/dashboard/todos.feature b/features/dashboard/todos.feature new file mode 100644 index 00000000000..1e7b1b50d64 --- /dev/null +++ b/features/dashboard/todos.feature @@ -0,0 +1,38 @@ +@dashboard +Feature: Dashboard Todos + Background: + 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 todos + And I visit dashboard todos page + + @javascript + 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 todos marked as done + + @javascript + Scenario: I filter by project + Given I filter by "Enterprise" + Then I should not see todos + + @javascript + Scenario: I filter by author + Given I filter by "John Doe" + 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 todos related to "Merge Requests" in the list + + @javascript + Scenario: I filter by action + Given I filter by "Mentioned" + Then I should not see todos related to "Assignments" in the list diff --git a/features/steps/dashboard/todos.rb b/features/steps/dashboard/todos.rb new file mode 100644 index 00000000000..9722a5a848c --- /dev/null +++ b/features/steps/dashboard/todos.rb @@ -0,0 +1,128 @@ +class Spinach::Features::DashboardTodos < Spinach::FeatureSteps + include SharedAuthentication + 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 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(: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 todos assigned to me' do + 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 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 you issue ##{issue.iid}", issue.title) + should_see_todo(4, "Mary Jane mentioned you on issue ##{issue.iid}", issue.title) + end + + step 'I mark the todo as done' do + page.within('.todo:nth-child(1)') do + click_link 'Done' + end + + expect(page).to have_content 'Todo was successfully marked as done.' + expect(page).to have_content 'To do 3' + expect(page).to have_content 'Done 1' + should_not_see_todo "John Doe assigned you merge request !#{merge_request.iid}" + end + + step 'I click on the "Done" tab' do + click_link 'Done 1' + end + + 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 you 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("#{Todo::MENTIONED}", from: '#action_id') + end + + step 'I should not see todos' do + expect(page).to have_content "You're all done!" + end + + 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 todos related to "Merge Requests" in the list' do + 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 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) + page.within(".todo: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 should_not_see_todo(title) + expect(page).not_to have_content title + end + + def john_doe + @john_doe ||= user_exists("John Doe", { username: "john_doe" }) + end + + 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 diff --git a/features/steps/shared/paths.rb b/features/steps/shared/paths.rb index 2c854ac7bf9..f4df4874d2f 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 todos page' do + visit dashboard_todos_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/todos.rb b/spec/factories/todos.rb new file mode 100644 index 00000000000..bd85b1d798a --- /dev/null +++ b/spec/factories/todos.rb @@ -0,0 +1,34 @@ +# == Schema Information +# +# Table name: todos +# +# 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 +# note_id :integer +# action :integer not null +# state :string not null +# created_at :datetime +# updated_at :datetime +# + +FactoryGirl.define do + factory :todo do + project + author + user + target factory: :issue + action { Todo::ASSIGNED } + + trait :assigned do + action { Todo::ASSIGNED } + end + + trait :mentioned do + action { Todo::MENTIONED } + end + end +end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index e6da3724d33..583937ca748 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(:todos).dependent(:destroy) } end describe 'validation' do diff --git a/spec/models/todo_spec.rb.rb b/spec/models/todo_spec.rb.rb new file mode 100644 index 00000000000..ac481bf9fbd --- /dev/null +++ b/spec/models/todo_spec.rb.rb @@ -0,0 +1,89 @@ +# == Schema Information +# +# Table name: todos +# +# 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 +# note_id :integer +# action :integer not null +# state :string not null +# created_at :datetime +# updated_at :datetime +# + +require 'spec_helper' + +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) } + 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 '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 proper message when action is an assigment' do + subject.action = Todo::ASSIGNED + + expect(subject.action_name).to eq 'assigned' + end + + it 'returns proper message when action is a mention' do + subject.action = Todo::MENTIONED + + expect(subject.action_name).to eq 'mentioned you on' + end + end + + describe '#body' do + before do + subject.target = build(:issue, title: 'Bugfix') + end + + it 'returns target title when note is blank' do + subject.note = 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.body).to eq 'quick fix' + end + end + + describe '#target_iid' do + 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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 47ce409fe4b..32d4f39b04a 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(: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 3a8daf28f5e..62b25709a5d 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -5,6 +5,7 @@ describe Issues::CloseService, services: true do let(:user2) { create(:user) } let(:issue) { create(:issue, assignee: user2) } let(:project) { issue.project } + let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) } before do project.team << [user, :master] @@ -32,6 +33,10 @@ describe Issues::CloseService, services: true do note = @issue.notes.last expect(note.note).to include "Status changed to closed" end + + it 'marks todos as done' do + expect(todo.reload).to be_done + end end context "external issue tracker" do @@ -42,6 +47,7 @@ describe Issues::CloseService, services: true do it { expect(@issue).to be_valid } it { expect(@issue).to be_opened } + 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 2148d091a57..5e7915db7e1 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,21 @@ 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 todo for new assignee' do + attributes = { + project: project, + author: user, + user: assignee, + target_id: @issue.id, + target_type: @issue.class.name, + action: Todo::ASSIGNED, + state: :pending + } + + expect(Todo.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..e579e49dfa7 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -80,6 +80,74 @@ describe Issues::UpdateService, services: true do end end + 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 todos as done' do + expect(todo.reload.done?).to eq true + end + end + + context 'when the description change' do + before do + update_issue({ description: 'Also please fix' }) + end + + it 'marks todos as done' do + expect(todo.reload.done?).to eq true + end + end + + context 'when is reassigned' do + before do + update_issue({ assignee: user2 }) + end + + it 'marks previous assignee todos as done' do + expect(todo.reload.done?).to eq true + end + + it 'creates a todo for new assignee' do + attributes = { + project: project, + author: user, + user: user2, + target_id: issue.id, + target_type: issue.class.name, + action: Todo::ASSIGNED, + state: :pending + } + + expect(Todo.where(attributes).count).to eq 1 + end + end + + context 'when the milestone change' do + before do + update_issue({ milestone: create(:milestone) }) + end + + it 'marks todos as done' do + expect(todo.reload.done?).to eq true + end + end + + context 'when the labels change' do + before do + update_issue({ label_ids: [label.id] }) + end + + it 'marks todos as done' do + expect(todo.reload.done?).to eq true + end + end + end + context 'when Issue has tasks' do before { update_issue({ description: "- [ ] Task 1\n- [ ] Task 2" }) } @@ -144,6 +212,5 @@ describe Issues::UpdateService, services: true do end end end - end end diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index 50d0c288790..8443a00e70c 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -5,6 +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!(:todo) { create(:todo, :assigned, user: user, project: project, target: merge_request, author: user2) } before do project.team << [user, :master] @@ -41,6 +42,10 @@ describe MergeRequests::CloseService, services: true do note = @merge_request.notes.last expect(note.note).to include 'Status changed to closed' end + + it 'marks todos as done' do + expect(todo.reload).to be_done + end 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..120f4d6a669 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,49 @@ 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 todos' do + attributes = { + project: project, + target_id: @merge_request.id, + target_type: @merge_request.class.name + } + + expect(Todo.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 todo for new assignee' do + attributes = { + project: project, + author: user, + user: assignee, + target_id: @merge_request.id, + target_type: @merge_request.class.name, + action: Todo::ASSIGNED, + state: :pending + } + + expect(Todo.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..99703c7a8ec 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -98,6 +98,84 @@ describe MergeRequests::UpdateService, services: true do end end + 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 todos as done' do + expect(pending_todo.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 todos as done' do + expect(pending_todo.reload).to be_done + end + end + + context 'when is reassigned' do + before do + update_merge_request({ assignee: user2 }) + end + + it 'marks previous assignee pending todos as done' do + expect(pending_todo.reload).to be_done + end + + it 'creates a pending todo for new assignee' do + attributes = { + project: project, + author: user, + user: user2, + target_id: merge_request.id, + target_type: merge_request.class.name, + action: Todo::ASSIGNED, + state: :pending + } + + expect(Todo.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 todos as done' do + expect(pending_todo.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 todos as done' do + expect(pending_todo.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 todos as done' do + expect(pending_todo.reload).to be_done + end + end + end + context 'when MergeRequest has tasks' do before { update_merge_request({ description: "- [ ] Task 1\n- [ ] Task 2" }) } @@ -130,6 +208,5 @@ describe MergeRequests::UpdateService, services: true do end end end - end end diff --git a/spec/services/notes/post_process_service_spec.rb b/spec/services/notes/post_process_service_spec.rb index 1a3f339bd64..d4c50f824c1 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) + Notes::PostProcessService.new(@note).execute end end diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb new file mode 100644 index 00000000000..dde4bde7dc2 --- /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 '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 todos as done' do + expect(todo.reload).to be_done + end + end + + context 'when the note does not change' do + before do + update_note({ note: 'Old note' }) + end + + it 'keep todos' do + expect(todo.reload).to be_pending + end + end + end + 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 -- cgit v1.2.3