From 0a27c129bc1ccab09be9f5e71ed194b55394caaf Mon Sep 17 00:00:00 2001
From: Felipe Artur <felipefac@gmail.com>
Date: Tue, 1 Jun 2021 16:31:14 -0300
Subject: [PATCH] Mark pending todo as done when resolving design discussions
Mark todos of user that resolved a design discussion as done
Changelog: added
---
app/finders/pending_todos_finder.rb | 9 ++++
app/models/todo.rb | 1 +
app/services/discussions/resolve_service.rb | 7 +++
app/services/todo_service.rb | 2 +
doc/user/project/issues/design_management.md | 3 ++
doc/user/todos.md | 1 +
spec/finders/pending_todos_finder_spec.rb | 28 +++++++++---
spec/models/todo_spec.rb | 12 +++++
.../discussions/resolve_service_spec.rb | 45 +++++++++++++++++++
9 files changed, 101 insertions(+), 7 deletions(-)
diff --git a/app/finders/pending_todos_finder.rb b/app/finders/pending_todos_finder.rb
index d79a2340379cf..509370b49a877 100644
--- a/app/finders/pending_todos_finder.rb
+++ b/app/finders/pending_todos_finder.rb
@@ -26,6 +26,7 @@ def execute
todos = by_project(todos)
todos = by_target_id(todos)
todos = by_target_type(todos)
+ todos = by_discussion(todos)
by_commit_id(todos)
end
@@ -60,4 +61,12 @@ def by_commit_id(todos)
todos
end
end
+
+ def by_discussion(todos)
+ if (discussion = params[:discussion])
+ todos.for_note(discussion.notes)
+ else
+ todos
+ end
+ end
end
diff --git a/app/models/todo.rb b/app/models/todo.rb
index 23685fb68e0dc..94a996038489b 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -61,6 +61,7 @@ class Todo < ApplicationRecord
scope :for_author, -> (author) { where(author: author) }
scope :for_user, -> (user) { where(user: user) }
scope :for_project, -> (projects) { where(project: projects) }
+ scope :for_note, -> (notes) { where(note: notes) }
scope :for_undeleted_projects, -> { joins(:project).merge(Project.without_deleted) }
scope :for_group, -> (group) { where(group: group) }
scope :for_type, -> (type) { where(target_type: type) }
diff --git a/app/services/discussions/resolve_service.rb b/app/services/discussions/resolve_service.rb
index 3b733023eaedb..baf14aa8a03f7 100644
--- a/app/services/discussions/resolve_service.rb
+++ b/app/services/discussions/resolve_service.rb
@@ -47,9 +47,16 @@ def resolve_discussion(discussion)
MergeRequests::ResolvedDiscussionNotificationService.new(project: project, current_user: current_user).execute(merge_request)
end
+ resolve_user_todos_for(discussion)
SystemNoteService.discussion_continued_in_issue(discussion, project, current_user, follow_up_issue) if follow_up_issue
end
+ def resolve_user_todos_for(discussion)
+ return unless discussion.for_design?
+
+ TodoService.new.resolve_todos_for_target(discussion, current_user)
+ end
+
def first_discussion
@first_discussion ||= discussions.first
end
diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb
index fc6543a8efc97..71bb813f384f3 100644
--- a/app/services/todo_service.rb
+++ b/app/services/todo_service.rb
@@ -316,6 +316,8 @@ def attributes_for_target(target)
attributes.merge!(target_id: nil, commit_id: target.id)
elsif target.is_a?(Issue)
attributes[:issue_type] = target.issue_type
+ elsif target.is_a?(Discussion)
+ attributes.merge!(target_type: nil, target_id: nil, discussion: target)
end
attributes
diff --git a/doc/user/project/issues/design_management.md b/doc/user/project/issues/design_management.md
index 8b67c7e03ce94..8c42a27b4719d 100644
--- a/doc/user/project/issues/design_management.md
+++ b/doc/user/project/issues/design_management.md
@@ -219,6 +219,9 @@ There are two ways to resolve/unresolve a Design thread:
+Resolving a discussion thread will also mark any pending to-do related to notes
+inside the thread as done. This is applicable only for to-dos owned by the user triggering the action.
+
Note that your resolved comment pins disappear from the Design to free up space for new discussions.
However, if you need to revisit or find a resolved discussion, all of your resolved threads are
available in the **Resolved Comment** area at the bottom of the right sidebar.
diff --git a/doc/user/todos.md b/doc/user/todos.md
index 695532abf9f80..4227f46dfa8eb 100644
--- a/doc/user/todos.md
+++ b/doc/user/todos.md
@@ -112,6 +112,7 @@ Actions that dismiss to-do items include:
- Changing the milestone
- Adding/removing a label
- Commenting on the issue
+- Resolving a [design discussion thread](project/issues/design_management.md#resolve-design-threads)
Your To-Do List is personal, and items are only marked as done if you take
action. If you close the issue or merge request, your to-do item is marked as
diff --git a/spec/finders/pending_todos_finder_spec.rb b/spec/finders/pending_todos_finder_spec.rb
index b17915f0d5921..f317d8b1633ef 100644
--- a/spec/finders/pending_todos_finder_spec.rb
+++ b/spec/finders/pending_todos_finder_spec.rb
@@ -3,8 +3,11 @@
require 'spec_helper'
RSpec.describe PendingTodosFinder do
- let(:user) { create(:user) }
- let(:user2) { create(:user) }
+ let_it_be(:user) { create(:user) }
+ let_it_be(:user2) { create(:user) }
+ let_it_be(:issue) { create(:issue) }
+ let_it_be(:note) { create(:note) }
+
let(:users) { [user, user2] }
describe '#execute' do
@@ -30,20 +33,16 @@
end
it 'supports retrieving of todos for a specific todo target' do
- issue = create(:issue)
- note = create(:note)
todo = create(:todo, :pending, user: user, target: issue)
create(:todo, :pending, user: user, target: note)
- todos = described_class.new(users, target_id: issue.id).execute
+ todos = described_class.new(users, target_id: issue.id, target_type: 'Issue').execute
expect(todos).to eq([todo])
end
it 'supports retrieving of todos for a specific target type' do
- issue = create(:issue)
- note = create(:note)
todo = create(:todo, :pending, user: user, target: issue)
create(:todo, :pending, user: user, target: note)
@@ -61,5 +60,20 @@
expect(todos).to eq([todo])
end
+
+ it 'supports retrieving of todos for specific discussion' do
+ first_discussion_note = create(:discussion_note_on_issue, noteable: issue, project: issue.project)
+ note_2 = create(:note, discussion_id: first_discussion_note.discussion_id)
+ note_3 = create(:note, discussion_id: first_discussion_note.discussion_id)
+ todo1 = create(:todo, :pending, target: issue, note: note_2, user: note_2.author)
+ todo2 = create(:todo, :pending, target: issue, note: note_3, user: note_3.author)
+ create(:todo, :pending, note: note, user: user)
+ discussion = Discussion.lazy_find(first_discussion_note.discussion_id)
+ users = [note_2.author, note_3.author, user]
+
+ todos = described_class.new(users, discussion: discussion).execute
+
+ expect(todos).to contain_exactly(todo1, todo2)
+ end
end
end
diff --git a/spec/models/todo_spec.rb b/spec/models/todo_spec.rb
index caa0a886abf35..651e2cf273f99 100644
--- a/spec/models/todo_spec.rb
+++ b/spec/models/todo_spec.rb
@@ -376,6 +376,18 @@
end
end
+ describe '.for_note' do
+ it 'returns todos that belongs to notes' do
+ note_1 = create(:note, noteable: issue, project: issue.project)
+ note_2 = create(:note, noteable: issue, project: issue.project)
+ todo_1 = create(:todo, note: note_1)
+ todo_2 = create(:todo, note: note_2)
+ create(:todo, note: create(:note))
+
+ expect(described_class.for_note([note_1, note_2])).to contain_exactly(todo_1, todo_2)
+ end
+ end
+
describe '.group_by_user_id_and_state' do
let_it_be(:user1) { create(:user) }
let_it_be(:user2) { create(:user) }
diff --git a/spec/services/discussions/resolve_service_spec.rb b/spec/services/discussions/resolve_service_spec.rb
index 2e30455eb0a7c..24de1d90526cc 100644
--- a/spec/services/discussions/resolve_service_spec.rb
+++ b/spec/services/discussions/resolve_service_spec.rb
@@ -121,5 +121,50 @@
service.execute
end
end
+
+ context 'when resolving a discussion' do
+ def resolve_discussion(discussion, user)
+ described_class.new(project, user, one_or_more_discussions: discussion).execute
+ end
+
+ context 'in a design' do
+ let_it_be(:design) { create(:design, :with_file, issue: create(:issue, project: project)) }
+ let_it_be(:user_1) { create(:user) }
+ let_it_be(:user_2) { create(:user) }
+ let_it_be(:discussion_1) { create(:diff_note_on_design, noteable: design, project: project, author: user_1).to_discussion }
+ let_it_be(:discussion_2) { create(:diff_note_on_design, noteable: design, project: project, author: user_2).to_discussion }
+
+ before do
+ project.add_developer(user_1)
+ project.add_developer(user_2)
+ end
+
+ context 'when user resolving discussion has open todos' do
+ let!(:user_1_todo_for_discussion_1) { create(:todo, :pending, user: user_1, target: design, note: discussion_1.notes.first, project: project) }
+ let!(:user_1_todo_2_for_discussion_1) { create(:todo, :pending, user: user_1, target: design, note: discussion_1.notes.first, project: project) }
+ let!(:user_1_todo_for_discussion_2) { create(:todo, :pending, user: user_1, target: design, note: discussion_2.notes.first, project: project) }
+ let!(:user_2_todo_for_discussion_1) { create(:todo, :pending, user: user_2, target: design, note: discussion_1.notes.first, project: project) }
+
+ it 'marks user todos for given discussion as done' do
+ resolve_discussion(discussion_1, user_1)
+
+ expect(user_1_todo_for_discussion_1.reload).to be_done
+ expect(user_1_todo_2_for_discussion_1.reload).to be_done
+ expect(user_1_todo_for_discussion_2.reload).to be_pending
+ expect(user_2_todo_for_discussion_1.reload).to be_pending
+ end
+ end
+ end
+
+ context 'in a merge request' do
+ let!(:user_todo_for_discussion) { create(:todo, :pending, user: user, target: merge_request, note: discussion.notes.first, project: project) }
+
+ it 'does not mark user todo as done' do
+ resolve_discussion(discussion, user)
+
+ expect(user_todo_for_discussion).to be_pending
+ end
+ end
+ end
end
end
--
GitLab