Merge branch 'graphql-bug-fix-get-todo-by-type' into 'master'

Fix broken todo GraphQL API filtering when filtering by type

See merge request gitlab-org/gitlab!34790

app/finders/todos_finder.rb

@@ -11,7 +11,7 @@
 #     author_id: integer
 #     project_id; integer
 #     state: 'pending' (default) or 'done'
-#     type: 'Issue' or 'MergeRequest'
+#     type: 'Issue' or 'MergeRequest' or ['Issue', 'MergeRequest']
 #
 
 class TodosFinder
@@ -40,13 +40,14 @@ class TodosFinder
 
   def execute
     return Todo.none if current_user.nil?
+    raise ArgumentError, invalid_type_message unless valid_types?
 
     items = current_user.todos
     items = by_action_id(items)
     items = by_action(items)
     items = by_author(items)
     items = by_state(items)
-    items = by_type(items)
+    items = by_types(items)
     items = by_group(items)
     # Filtering by project HAS TO be the last because we use
     # the project IDs yielded by the todos query thus far
@@ -123,12 +124,16 @@ class TodosFinder
     end
   end
 
-  def type?
-    type.present? && self.class.todo_types.include?(type)
+  def types
+    @types ||= Array(params[:type]).reject(&:blank?)
   end
 
-  def type
-    params[:type]
+  def valid_types?
+    types.all? { |type| self.class.todo_types.include?(type) }
+  end
+
+  def invalid_type_message
+    _("Unsupported todo type passed. Supported todo types are: %{todo_types}") % { todo_types: self.class.todo_types.to_a.join(', ') }
   end
 
   def sort(items)
@@ -193,9 +198,9 @@ class TodosFinder
     items.with_states(params[:state])
   end
 
-  def by_type(items)
-    if type?
-      items.for_type(type)
+  def by_types(items)
+    if types.any?
+      items.for_type(types)
     else
       items
     end

changelogs/unreleased/graphql-bug-fix-get-todo-by-type.yml

+---
+title: Fix broken todo GraphQL API filtering when filtering by type
+merge_request: 34790
+author:
+type: fixed

locale/gitlab.pot

@@ -24460,6 +24460,9 @@ msgstr ""
 msgid "Unsubscribes from this %{quick_action_target}."
 msgstr ""
 
+msgid "Unsupported todo type passed. Supported todo types are: %{todo_types}"
+msgstr ""
+
 msgid "Until"
 msgstr ""
 

spec/finders/todos_finder_spec.rb

@@ -37,16 +37,63 @@ RSpec.describe TodosFinder do
         end
 
         context 'when filtering by type' do
-          it 'returns correct todos when filtered by a type' do
+          it 'returns todos by type when filtered by a single type' do
             todos = finder.new(user, { type: 'Issue' }).execute
 
             expect(todos).to match_array([todo1])
           end
 
-          it 'returns the correct todos when filtering for multiple types' do
+          it 'returns todos by type when filtered by multiple types' do
+            design_todo = create(:todo, user: user, group: group, target: create(:design))
+
             todos = finder.new(user, { type: %w[Issue MergeRequest] }).execute
 
-            expect(todos).to match_array([todo1, todo2])
+            expect(todos).to contain_exactly(todo1, todo2)
+            expect(todos).not_to include(design_todo)
+          end
+
+          it 'returns all todos when type is nil' do
+            todos = finder.new(user, { type: nil }).execute
+
+            expect(todos).to contain_exactly(todo1, todo2)
+          end
+
+          it 'returns all todos when type is an empty collection' do
+            todos = finder.new(user, { type: [] }).execute
+
+            expect(todos).to contain_exactly(todo1, todo2)
+          end
+
+          it 'returns all todos when type is blank' do
+            todos = finder.new(user, { type: '' }).execute
+
+            expect(todos).to contain_exactly(todo1, todo2)
+          end
+
+          it 'returns todos by type when blank type is in type collection' do
+            todos = finder.new(user, { type: ['', 'MergeRequest'] }).execute
+
+            expect(todos).to contain_exactly(todo2)
+          end
+
+          it 'returns todos of all types when only blanks are in a collection' do
+            todos = finder.new(user, { type: ['', ''] }).execute
+
+            expect(todos).to contain_exactly(todo1, todo2)
+          end
+
+          it 'returns all todos when no type param' do
+            todos = finder.new(user).execute
+
+            expect(todos).to contain_exactly(todo1, todo2)
+          end
+
+          it 'raises an argument error when invalid type is passed' do
+            create(:todo, user: user, group: group, target: create(:design))
+
+            todos_finder = finder.new(user, { type: %w[Issue MergeRequest NotAValidType] })
+
+            expect { todos_finder.execute }.to raise_error(ArgumentError)
           end
         end
 

spec/graphql/resolvers/todo_resolver_spec.rb

@@ -10,9 +10,9 @@ RSpec.describe Resolvers::TodoResolver do
     let_it_be(:author1) { create(:user) }
     let_it_be(:author2) { create(:user) }
 
-    let_it_be(:todo1) { create(:todo, user: current_user, target_type: 'MergeRequest', state: :pending, action: Todo::MENTIONED, author: author1) }
-    let_it_be(:todo2) { create(:todo, user: current_user, state: :done, action: Todo::ASSIGNED, author: author2) }
-    let_it_be(:todo3) { create(:todo, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1) }
+    let_it_be(:merge_request_todo_pending) { create(:todo, user: current_user, target_type: 'MergeRequest', state: :pending, action: Todo::MENTIONED, author: author1) }
+    let_it_be(:issue_todo_done) { create(:todo, user: current_user, state: :done, action: Todo::ASSIGNED, author: author2) }
+    let_it_be(:issue_todo_pending) { create(:todo, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1) }
 
     it 'calls TodosFinder' do
       expect_next_instance_of(TodosFinder) do |finder|
@@ -23,22 +23,30 @@ RSpec.describe Resolvers::TodoResolver do
     end
 
     context 'when using no filter' do
-      it 'returns expected todos' do
-        expect(resolve_todos).to contain_exactly(todo1, todo3)
+      it 'returns pending todos' do
+        expect(resolve_todos).to contain_exactly(merge_request_todo_pending, issue_todo_pending)
       end
     end
 
     context 'when using filters' do
       it 'returns the todos for multiple states' do
-        todos = resolve_todos({ state: [:done, :pending] })
+        todos = resolve_todos(state: [:done, :pending])
 
-        expect(todos).to contain_exactly(todo1, todo2, todo3)
+        expect(todos).to contain_exactly(merge_request_todo_pending, issue_todo_done, issue_todo_pending)
       end
 
-      it 'returns the todos for multiple types' do
-        todos = resolve_todos({ type: %w[Issue MergeRequest] })
+      it 'returns the todos for multiple filters' do
+        design_todo_pending = create(:todo, target_type: 'DesignManagement::Design', user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
 
-        expect(todos).to contain_exactly(todo1, todo3)
+        todos = resolve_todos(type: ['MergeRequest', 'DesignManagement::Design'])
+
+        expect(todos).to contain_exactly(merge_request_todo_pending, design_todo_pending)
+      end
+
+      it 'returns the todos for single filter' do
+        todos = resolve_todos(type: 'MergeRequest')
+
+        expect(todos).to contain_exactly(merge_request_todo_pending)
       end
 
       it 'returns the todos for multiple groups' do
@@ -53,7 +61,7 @@ RSpec.describe Resolvers::TodoResolver do
         todo5 = create(:todo, group: group2, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
         create(:todo, group: group3, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
 
-        todos = resolve_todos({ group_id: [group2.id, group1.id] })
+        todos = resolve_todos(group_id: [group2.id, group1.id])
 
         expect(todos).to contain_exactly(todo4, todo5)
       end
@@ -61,20 +69,19 @@ RSpec.describe Resolvers::TodoResolver do
       it 'returns the todos for multiple authors' do
         author3 = create(:user)
 
-        todo4 = create(:todo, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author2)
         create(:todo, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author3)
 
-        todos = resolve_todos({ author_id: [author2.id, author1.id] })
+        todos = resolve_todos(author_id: [author2.id, author1.id])
 
-        expect(todos).to contain_exactly(todo1, todo3, todo4)
+        expect(todos).to contain_exactly(merge_request_todo_pending, issue_todo_pending)
       end
 
       it 'returns the todos for multiple actions' do
         create(:todo, user: current_user, state: :pending, action: Todo::DIRECTLY_ADDRESSED, author: author1)
 
-        todos = resolve_todos({ action: [Todo::MENTIONED, Todo::ASSIGNED] })
+        todos = resolve_todos(action: [Todo::MENTIONED, Todo::ASSIGNED])
 
-        expect(todos).to contain_exactly(todo1, todo3)
+        expect(todos).to contain_exactly(merge_request_todo_pending, issue_todo_pending)
       end
 
       it 'returns the todos for multiple projects' do
@@ -86,7 +93,7 @@ RSpec.describe Resolvers::TodoResolver do
         todo5 = create(:todo, project: project2, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
         create(:todo, project: project3, user: current_user, state: :pending, action: Todo::ASSIGNED, author: author1)
 
-        todos = resolve_todos({ project_id: [project2.id, project1.id] })
+        todos = resolve_todos(project_id: [project2.id, project1.id])
 
         expect(todos).to contain_exactly(todo4, todo5)
       end