Merge branch 'backend-issue_34239' into 'master'

Expose blocked by issue on IssueEntity

See merge request gitlab-org/gitlab!25723

app/views/projects/issues/_discussion.html.haml

@@ -7,7 +7,7 @@
 
 %section.issuable-discussion.js-vue-notes-event
   #js-vue-notes{ data: { notes_data: notes_data(@issue).to_json,
-    noteable_data: serialize_issuable(@issue),
+    noteable_data: serialize_issuable(@issue, with_blocking_issues: Feature.enabled?(:prevent_closing_blocked_issues, @issue.project)),
     noteable_type: 'Issue',
     target_type: 'issue',
     current_user_data: UserSerializer.new.represent(current_user, {only_path: true}, CurrentUserEntity).to_json } }

ee/app/models/ee/issue.rb

@@ -65,6 +65,20 @@ module EE
       project.feature_available?(:multiple_issue_assignees)
     end
 
+    def blocked?
+      blocking_issues_ids.any?
+    end
+
+    # Used on EE::IssueEntity to expose blocking issues URLs
+    def blocked_by_issues(user)
+      return ::Issue.none unless blocked?
+
+      issues =
+        ::IssuesFinder.new(user).execute.where(id: blocking_issues_ids)
+
+      issues.preload(project: [:route, { namespace: [:route] }])
+    end
+
     # override
     def subscribed_without_subscriptions?(user, *)
       # TODO: this really shouldn't be necessary, because the support
@@ -196,6 +210,10 @@ module EE
 
     private
 
+    def blocking_issues_ids
+      @blocking_issues_ids ||= ::IssueLink.blocking_issue_ids_for(self)
+    end
+
     def update_generic_alert_title
       update(title: "#{title} #{iid}")
     end

ee/app/models/issue_link.rb

@@ -20,39 +20,51 @@ class IssueLink < ApplicationRecord
 
   enum link_type: { TYPE_RELATES_TO => 0, TYPE_BLOCKS => 1, TYPE_IS_BLOCKED_BY => 2 }
 
-  def self.inverse_link_type(type)
-    case type
-    when TYPE_BLOCKS
-      TYPE_IS_BLOCKED_BY
-    when TYPE_IS_BLOCKED_BY
-      TYPE_BLOCKS
-    else
-      type
+  class << self
+    def inverse_link_type(type)
+      case type
+      when TYPE_BLOCKS
+        TYPE_IS_BLOCKED_BY
+      when TYPE_IS_BLOCKED_BY
+        TYPE_BLOCKS
+      else
+        type
+      end
+    end
+
+    def blocked_issue_ids(issue_ids)
+      blocked_and_blocking_issues_union(issue_ids).pluck(:blocked_issue_id)
     end
-  end
 
-  def self.blocked_issue_ids(issue_ids)
-    from_union([
-      blocked_issues(issue_ids, IssueLink::TYPE_BLOCKS),
-      blocked_issues(issue_ids, IssueLink::TYPE_IS_BLOCKED_BY)
-    ]).pluck(:issue_id)
+    def blocking_issue_ids_for(issue)
+      blocked_and_blocking_issues_union(issue.id).pluck(:blocking_issue_id)
+    end
   end
 
   private
 
-  def self.blocked_issues(issue_ids, link_type)
-    if link_type == IssueLink::TYPE_BLOCKS
-      blocked_key = :target_id
-      blocking_key = :source_id
-    else
-      blocked_key = :source_id
-      blocking_key = :target_id
+  class << self
+    def blocked_and_blocking_issues_union(issue_ids)
+      from_union([
+        blocked_or_blocking_issues(issue_ids, IssueLink::TYPE_BLOCKS),
+        blocked_or_blocking_issues(issue_ids, IssueLink::TYPE_IS_BLOCKED_BY)
+      ])
     end
 
-    select("#{blocked_key} as issue_id")
-      .where(link_type: link_type).where(blocked_key => issue_ids)
-      .joins("INNER JOIN issues ON issues.id = issue_links.#{blocking_key}")
-      .where('issues.state_id' => Issuable::STATE_ID_MAP[:opened])
+    def blocked_or_blocking_issues(issue_ids, link_type)
+      if link_type == IssueLink::TYPE_BLOCKS
+        blocked_key = :target_id
+        blocking_key = :source_id
+      else
+        blocked_key = :source_id
+        blocking_key = :target_id
+      end
+
+      select("#{blocked_key} as blocked_issue_id, #{blocking_key} as blocking_issue_id")
+        .where(link_type: link_type).where(blocked_key => issue_ids)
+        .joins("INNER JOIN issues ON issues.id = issue_links.#{blocking_key}")
+        .where('issues.state_id' => Issuable::STATE_ID_MAP[:opened])
+    end
   end
 
   def check_self_relation

ee/app/serializers/ee/issue_entity.rb

@@ -5,6 +5,17 @@ module EE
 
     prepended do
       expose :weight, if: ->(issue, _) { issue.supports_weight? }
+
+      with_options if: -> (_, options) { options[:with_blocking_issues] } do
+        expose :blocked?, as: :blocked
+
+        expose :blocked_by_issues do |issue|
+          issues = issue.blocked_by_issues(request.current_user)
+          serializer_options = options.merge(only: [:id, :web_url])
+
+          ::IssueEntity.represent(issues, serializer_options)
+        end
+      end
     end
   end
 end

ee/spec/models/issue_link_spec.rb

@@ -63,6 +63,20 @@ describe IssueLink do
     end
   end
 
+  describe '.blocking_issue_ids_for' do
+    it 'returns blocking issue ids' do
+      issue = create(:issue)
+      blocking_issue = create(:issue, project: issue.project)
+      blocked_by_issue = create(:issue, project: issue.project)
+      create(:issue_link, source: blocking_issue, target: issue, link_type: IssueLink::TYPE_BLOCKS)
+      create(:issue_link, source: issue, target: blocked_by_issue, link_type: IssueLink::TYPE_IS_BLOCKED_BY)
+
+      blocking_ids = described_class.blocking_issue_ids_for(issue)
+
+      expect(blocking_ids).to match_array([blocking_issue.id, blocked_by_issue.id])
+    end
+  end
+
   describe '.inverse_link_type' do
     it 'returns reverse type of link' do
       expect(described_class.inverse_link_type('relates_to')).to eq 'relates_to'

ee/spec/models/issue_spec.rb

@@ -614,6 +614,51 @@ describe Issue do
     end
   end
 
+  describe "#blocked_by_issues" do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:project) { create(:project) }
+    let_it_be(:issue) { create(:issue, project: project) }
+    let_it_be(:blocking_issue) { create(:issue, project: project) }
+    let_it_be(:other_project_blocking_issue) { create(:issue) }
+    let_it_be(:blocked_by_issue) { create(:issue, project: project) }
+    let_it_be(:confidential_blocked_by_issue) { create(:issue, :confidential, project: project) }
+    let_it_be(:related_issue) { create(:issue, project: project) }
+    let_it_be(:closed_blocking_issue) { create(:issue, project: project, state: :closed) }
+
+    before_all do
+      create(:issue_link, source: blocking_issue, target: issue, link_type: IssueLink::TYPE_BLOCKS)
+      create(:issue_link, source: other_project_blocking_issue, target: issue, link_type: IssueLink::TYPE_BLOCKS)
+      create(:issue_link, source: issue, target: blocked_by_issue, link_type: IssueLink::TYPE_IS_BLOCKED_BY)
+      create(:issue_link, source: issue, target: confidential_blocked_by_issue, link_type: IssueLink::TYPE_IS_BLOCKED_BY)
+      create(:issue_link, source: issue, target: related_issue, link_type: IssueLink::TYPE_RELATES_TO)
+      create(:issue_link, source: closed_blocking_issue, target: issue, link_type: IssueLink::TYPE_BLOCKS)
+    end
+
+    context 'when user can read issues' do
+      it 'returns blocked issues' do
+        project.add_developer(user)
+        other_project_blocking_issue.project.add_developer(user)
+
+        expect(issue.blocked_by_issues(user)).to match_array([blocking_issue, blocked_by_issue, other_project_blocking_issue, confidential_blocked_by_issue])
+      end
+    end
+
+    context 'when user cannot read issues' do
+      it 'returns empty array' do
+        expect(issue.blocked_by_issues(user)).to be_empty
+      end
+    end
+
+    context 'when user can read some issues' do
+      it 'returns issues that user can read' do
+        guest = create(:user)
+        project.add_guest(guest)
+
+        expect(issue.blocked_by_issues(guest)).to match_array([blocking_issue, blocked_by_issue])
+      end
+    end
+  end
+
   it_behaves_like 'having health status'
 
   describe '#service_desk?' do

ee/spec/requests/projects/issues_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::IssuesController do
+  let_it_be(:issue) { create(:issue) }
+  let_it_be(:project) { issue.project }
+  let_it_be(:user) { issue.author }
+  let_it_be(:blocking_issue) { create(:issue, project: project) }
+  let_it_be(:blocked_by_issue) { create(:issue, project: project) }
+
+  before do
+    login_as(user)
+  end
+
+  describe 'GET #show' do
+    def get_show
+      get project_issue_path(project, issue)
+    end
+
+    context 'with blocking issues' do
+      before do
+        stub_feature_flags(prevent_closing_blocked_issues: true)
+
+        get_show # Warm the cache
+      end
+
+      it 'does not cause extra queries when multiple blocking issues are present' do
+        create(:issue_link, source: blocking_issue, target: issue, link_type: IssueLink::TYPE_BLOCKS)
+
+        control = ActiveRecord::QueryRecorder.new { get_show }
+
+        other_project_issue = create(:issue)
+        other_project_issue.project.add_developer(user)
+        create(:issue_link, source: issue, target: other_project_issue, link_type: IssueLink::TYPE_IS_BLOCKED_BY)
+
+        expect { get_show }.not_to exceed_query_limit(control)
+      end
+    end
+  end
+end

ee/spec/serializers/ee/issue_entity_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe IssueEntity do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:resource) { create(:issue, project: project) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:blocking_issue) { create(:issue, project: project) }
+  let_it_be(:blocked_issue) { create(:issue, project: project) }
+  let(:request) { double('request', current_user: user) }
+
+  before_all do
+    project.add_developer(user)
+    create(:issue_link, source: blocking_issue, target: blocked_issue, link_type: IssueLink::TYPE_BLOCKS)
+  end
+
+  subject { described_class.new(resource, request: request).as_json }
+
+  context 'when with_blocking_issues option is not present' do
+    it 'exposes blocking issues' do
+      expect(subject).not_to include(:blocked)
+      expect(subject).not_to include(:blocked_by_issues)
+    end
+  end
+
+  context 'when with_blocking_issues option is present' do
+    subject { described_class.new(resource, request: request, with_blocking_issues: true).as_json }
+
+    it 'exposes blocking issues' do
+      expect(subject).to include(:blocked)
+      expect(subject).to include(:blocked_by_issues)
+    end
+
+    it 'exposes only id and web_path' do
+      response = described_class.new(blocked_issue, request: request, with_blocking_issues: true).as_json
+
+      expect(response[:blocked_by_issues].first.keys).to match_array([:id, :web_url])
+    end
+  end
+end