Merge branch '13543-todos-api-designs' into 'master'

Return Todos for Designs via the REST API

See merge request gitlab-org/gitlab!16885

ee/app/models/design_management/design.rb

@@ -47,6 +47,9 @@ module DesignManagement
       joins(join.join_sources).where(actions[:event].not_eq(deletion))
     end
 
+    # Scope called by our REST API to avoid N+1 problems
+    scope :with_api_entity_associations, -> { preload(:issue) }
+
     # A design is current if the most recent event is not a deletion
     scope :current, -> { visible_at_version(nil) }
 

ee/changelogs/unreleased/13543-todos-api-designs.yml

+---
+title: Return Todos for Designs via the REST API
+merge_request: 16885
+author:
+type: added

ee/lib/ee/api/entities.rb

@@ -185,8 +185,10 @@ module EE
       end
 
       module Todo
+        extend ::Gitlab::Utils::Override
         extend ActiveSupport::Concern
 
+        override :todo_target_class
         def todo_target_class(target_type)
           super
         rescue NameError
@@ -194,11 +196,35 @@ module EE
           # see also https://gitlab.com/gitlab-org/gitlab-foss/issues/59719
           ::EE::API::Entities.const_get(target_type, false)
         end
+
+        override :todo_target_url
+        def todo_target_url(todo)
+          return super unless todo.target_type == ::DesignManagement::Design.name
+
+          design = todo.target
+          path_options = {
+            anchor: todo_target_anchor(todo),
+            vueroute: design.filename
+          }
+
+          ::Gitlab::Routing.url_helpers.designs_project_issue_url(design.project, design.issue, path_options)
+        end
       end
 
       ########################
       # EE-specific entities #
       ########################
+      module DesignManagement
+        class Design < Grape::Entity
+          expose :id
+          expose :project_id
+          expose :filename
+          expose :image_url do |design|
+            ::Gitlab::UrlBuilder.build(design)
+          end
+        end
+      end
+
       class ProjectPushRule < Grape::Entity
         extend EntityHelpers
         expose :id, :project_id, :created_at

ee/lib/ee/api/todos.rb

@@ -21,10 +21,12 @@ module EE
 
           override :find_todos
           def find_todos
-            # Todos for Designs are intentionally not supported yet.
-            # https://gitlab.com/gitlab-org/gitlab/issues/13543
-            # https://gitlab.com/gitlab-org/gitlab/issues/13494#note_203518559
-            super.where.not(target_type: ::DesignManagement::Design.name) # rubocop: disable CodeReuse/ActiveRecord
+            todos = super
+
+            return todos if ::Feature.enabled?(:design_management_todos_api, default_enabled: true)
+
+            # Exclude Design Todos if the feature is disabled
+            todos.where.not(target_type: ::DesignManagement::Design.name) # rubocop: disable CodeReuse/ActiveRecord
           end
         end
 

ee/lib/ee/gitlab/url_builder.rb

+# frozen_string_literal: true
+
+module EE
+  module Gitlab
+    module UrlBuilder
+      extend ::Gitlab::Utils::Override
+
+      override :url
+      def url
+        return project_design_url(object.project, object) if object.is_a?(DesignManagement::Design)
+
+        super
+      end
+    end
+  end
+end

ee/spec/lib/ee/api/entities/design_management/design_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EE::API::Entities::DesignManagement::Design do
+  set(:design) { create(:design) }
+  let(:entity) { described_class.new(design, request: double) }
+
+  subject { entity.as_json }
+
+  it 'has the correct attributes' do
+    expect(subject).to eq({
+      id: design.id,
+      project_id: design.project_id,
+      filename: design.filename,
+      image_url: ::Gitlab::UrlBuilder.build(design)
+    })
+  end
+end

ee/spec/lib/ee/gitlab/url_builder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::UrlBuilder do
+  describe '.build' do
+    context 'when passing a DesignManagement::Design' do
+      it 'returns a proper URL' do
+        design = build_stubbed(:design)
+
+        url = described_class.build(design)
+
+        expect(url).to eq "#{Settings.gitlab['url']}/#{design.project.full_path}/-/designs/#{design.id}"
+      end
+    end
+  end
+end

ee/spec/requests/api/todos_spec.rb

@@ -34,6 +34,12 @@ describe API::Todos do
              note: create(:note, project: project, note: "I am note, hear me roar"))
     end
 
+    shared_examples 'an endpoint that responds with success' do
+      it do
+        expect(response.status).to eq(200)
+      end
+    end
+
     context 'when there is an Epic Todo' do
       let!(:epic_todo) { create_todo_for_new_epic }
 
@@ -41,9 +47,7 @@ describe API::Todos do
         get api('/todos', personal_access_token: pat)
       end
 
-      it 'responds without error' do
-        expect(response.status).to eq(200)
-      end
+      it_behaves_like 'an endpoint that responds with success'
 
       it 'avoids N+1 queries', :request_store do
         create_todo_for_new_epic
@@ -65,16 +69,43 @@ describe API::Todos do
     context 'when there is a Design Todo' do
       let!(:design_todo) { create_todo_for_mentioned_in_design }
 
-      before do
+      def api_request
         get api('/todos', personal_access_token: pat)
       end
 
-      it 'responds without error' do
-        expect(response.status).to eq(200)
+      context 'when the feature is enabled' do
+        before do
+          api_request
+        end
+
+        it_behaves_like 'an endpoint that responds with success'
+
+        it 'avoids N+1 queries', :request_store do
+          control = ActiveRecord::QueryRecorder.new { api_request }
+
+          create_todo_for_mentioned_in_design
+
+          expect { api_request }.not_to exceed_query_limit(control)
+        end
+
+        it 'includes the Design Todo in the response' do
+          expect(json_response).to include(
+            a_hash_including('id' => design_todo.id)
+          )
+        end
       end
 
-      it 'does not include the Design Todo in the response' do
-        expect(json_response).to be_empty
+      context 'when the feature is disabled' do
+        before do
+          stub_feature_flags(design_management_todos_api: false)
+          api_request
+        end
+
+        it_behaves_like 'an endpoint that responds with success'
+
+        it 'does not include the Design Todo in the response' do
+          expect(json_response).to be_empty
+        end
       end
     end
   end

lib/api/entities.rb

@@ -965,13 +965,7 @@ module API
       end
 
       expose :target_url do |todo, options|
-        target_type   = todo.target_type.underscore
-        target_url    = "#{todo.parent.class.to_s.underscore}_#{target_type}_url"
-        target_anchor = "note_#{todo.note_id}" if todo.note_id?
-
-        Gitlab::Routing
-          .url_helpers
-          .public_send(target_url, todo.parent, todo.target, anchor: target_anchor) # rubocop:disable GitlabSecurity/PublicSend
+        todo_target_url(todo)
       end
 
       expose :body
@@ -983,6 +977,19 @@ module API
         # see also https://gitlab.com/gitlab-org/gitlab-foss/issues/59719
         ::API::Entities.const_get(target_type, false)
       end
+
+      def todo_target_url(todo)
+        target_type = todo.target_type.underscore
+        target_url = "#{todo.parent.class.to_s.underscore}_#{target_type}_url"
+
+        Gitlab::Routing
+          .url_helpers
+          .public_send(target_url, todo.parent, todo.target, anchor: todo_target_anchor(todo)) # rubocop:disable GitlabSecurity/PublicSend
+      end
+
+      def todo_target_anchor(todo)
+        "note_#{todo.note_id}" if todo.note_id?
+      end
     end
 
     class NamespaceBasic < Grape::Entity

lib/gitlab/url_builder.rb

@@ -81,3 +81,5 @@ module Gitlab
     end
   end
 end
+
+::Gitlab::UrlBuilder.prepend_if_ee('EE::Gitlab::UrlBuilder')