Merge branch 'issue_227691' into 'master'

Mark pending todos as done when resolving design discussions

See merge request gitlab-org/gitlab!62951

app/finders/pending_todos_finder.rb

@@ -26,6 +26,7 @@ class PendingTodosFinder
     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 @@ class PendingTodosFinder
       todos
     end
   end
+
+  def by_discussion(todos)
+    if (discussion = params[:discussion])
+      todos.for_note(discussion.notes)
+    else
+      todos
+    end
+  end
 end

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) }

app/services/discussions/resolve_service.rb

@@ -47,9 +47,16 @@ module Discussions
         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

app/services/todo_service.rb

@@ -316,6 +316,8 @@ class TodoService
       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

doc/user/project/issues/design_management.md

@@ -219,6 +219,9 @@ There are two ways to resolve/unresolve a Design thread:
 
   ![Resolve checkbox](img/resolve_design-discussion_checkbox_v13_1.png)
 
+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.

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

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 @@ RSpec.describe PendingTodosFinder do
     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 @@ RSpec.describe PendingTodosFinder do
 
       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

spec/models/todo_spec.rb

@@ -376,6 +376,18 @@ RSpec.describe Todo do
     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) }

spec/services/discussions/resolve_service_spec.rb

@@ -121,5 +121,50 @@ RSpec.describe Discussions::ResolveService do
         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