Merge branch '2001-related-issues' into 'master'

Related issues [BE]

Closes #2001

See merge request !1719

app/controllers/projects/issue_links_controller.rb

+module Projects
+  class IssueLinksController < ApplicationController
+    before_action :authorize_admin_issue_link!, only: [:create, :destroy]
+
+    def index
+      render json: issues
+    end
+
+    def create
+      create_params = params.slice(:issue_references)
+      result = IssueLinks::CreateService.new(issue, current_user, create_params).execute
+
+      render json: { message: result[:message], issues: issues }, status: result[:http_status]
+    end
+
+    def destroy
+      issue_link = IssueLink.find(params[:id])
+
+      return render_403 unless can?(current_user, :admin_issue_link, issue_link.target.project)
+
+      IssueLinks::DestroyService.new(issue_link, current_user).execute
+
+      render json: { issues: issues }
+    end
+
+    private
+
+    def issues
+      IssueLinks::ListService.new(issue, current_user).execute
+    end
+
+    def authorize_admin_issue_link!
+      render_403 unless can?(current_user, :admin_issue_link, @project)
+    end
+
+    def issue
+      @issue ||=
+        IssuesFinder.new(current_user, project_id: @project.id)
+                    .execute
+                    .find_by!(iid: params[:issue_id])
+    end
+  end
+end

app/finders/issuable_finder.rb

@@ -16,6 +16,7 @@
 #     label_name: string
 #     sort: string
 #     non_archived: boolean
+#     feature_availability_check: boolean (default: true)
 #     iids: integer[]
 #
 class IssuableFinder
@@ -25,11 +26,15 @@ class IssuableFinder
   ARRAY_PARAMS = { label_name: [], iids: [] }.freeze
   VALID_PARAMS = (SCALAR_PARAMS + [ARRAY_PARAMS]).freeze
+  DEFAULT_PARAMS = {
+    feature_availability_check: true
+  }.freeze
+
   attr_accessor :current_user, :params
   def initialize(current_user, params = {})
     @current_user = current_user
-    @params = params
+    @params = DEFAULT_PARAMS.merge(params).with_indifferent_access
   end
   def execute
@@ -126,7 +131,20 @@ def projects(items = nil)
         ProjectsFinder.new(current_user: current_user, project_ids_relation: item_project_ids(items)).execute
       end
-    @projects = projects.with_feature_available_for_user(klass, current_user).reorder(nil)
+    # Querying through feature availability for an user is expensive
+    # (i.e. https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1719#note_31406525),
+    # and there are cases which a project level access check should be enough.
+    # In any case, `feature_availability_check` param should be kept with `true`
+    # by default.
+    #
+    projects =
+      if params[:feature_availability_check]
+        projects.with_feature_available_for_user(klass, current_user)
+      else
+        projects
+      end
+
+    @projects = projects.reorder(nil)
   end
   def search

app/models/issue_link.rb

+class IssueLink < ActiveRecord::Base
+  belongs_to :source, class_name: 'Issue'
+  belongs_to :target, class_name: 'Issue'
+
+  validates :source, presence: true
+  validates :target, presence: true
+  validates :source, uniqueness: { scope: :target_id, message: 'is already related' }
+  validate :check_self_relation
+
+  private
+
+  def check_self_relation
+    return unless source && target
+
+    if source == target
+      errors.add(:source, 'cannot be related to itself')
+    end
+  end
+end

app/models/license.rb

@@ -6,11 +6,13 @@ class License < ActiveRecord::Base
   GEO_FEATURE = 'GitLab_Geo'.freeze
   AUDITOR_USER_FEATURE = 'GitLab_Auditor_User'.freeze
   SERVICE_DESK_FEATURE = 'GitLab_ServiceDesk'.freeze
+  RELATED_ISSUES_FEATURE = 'RelatedIssues'.freeze
   FEATURE_CODES = {
     geo: GEO_FEATURE,
     auditor_user: AUDITOR_USER_FEATURE,
     service_desk: SERVICE_DESK_FEATURE,
+    related_issues: RELATED_ISSUES_FEATURE,
     # Features that make sense to Namespace:
     deploy_board: DEPLOY_BOARD_FEATURE,
     file_lock: FILE_LOCK_FEATURE
@@ -22,7 +24,7 @@ class License < ActiveRecord::Base
   EARLY_ADOPTER_PLAN = 'early_adopter'.freeze
   EES_FEATURES = [
-    # ..
+    { RELATED_ISSUES_FEATURE => 1 }
   ].freeze
   EEP_FEATURES = [

app/models/system_note_metadata.rb

@@ -3,7 +3,7 @@ class SystemNoteMetadata < ActiveRecord::Base
     commit description merge confidential visible label assignee cross_reference
     title time_tracking branch milestone discussion task moved opened closed merged
     outdated
-    approved unapproved
+    approved unapproved relate unrelate
   ].freeze
   validates :note, presence: true

app/policies/ee/project_policy.rb

@@ -22,6 +22,11 @@ def disabled_features!
         cannot! :create_note
         cannot! :read_project
       end
+
+      unless project.feature_available?(:related_issues)
+        cannot! :read_issue_link
+        cannot! :admin_issue_link
+      end
     end
   end
 end

app/policies/project_policy.rb

@@ -55,6 +55,9 @@ def guest_access!
       can! :read_pipeline_schedule
       can! :read_build
     end
+
+    # EE-only
+    can! :read_issue_link
   end
   def reporter_access!
@@ -79,6 +82,9 @@ def reporter_access!
     if project.feature_available?(:deploy_board) || Rails.env.development?
       can! :read_deploy_board
     end
+
+    # EE-only
+    can! :admin_issue_link
   end
   # Permissions given when an user is team member of a project
@@ -321,5 +327,8 @@ def base_readonly_access!
     # NOTE: may be overridden by IssuePolicy
     can! :read_issue
+
+    # EE-only
+    can! :read_issue_link
   end
 end

app/services/issue_links/create_service.rb

+module IssueLinks
+  class CreateService < BaseService
+    def initialize(issue, user, params)
+      @issue, @current_user, @params = issue, user, params.dup
+    end
+
+    def execute
+      if referenced_issues.blank?
+        return error('No Issue found for given reference', 401)
+      end
+
+      create_issue_links
+      success
+    end
+
+    private
+
+    def create_issue_links
+      referenced_issues.each do |referenced_issue|
+        create_notes(referenced_issue) if relate_issues(referenced_issue)
+      end
+    end
+
+    def relate_issues(referenced_issue)
+      IssueLink.create(source: @issue, target: referenced_issue)
+    end
+
+    def create_notes(referenced_issue)
+      SystemNoteService.relate_issue(@issue, referenced_issue, current_user)
+      SystemNoteService.relate_issue(referenced_issue, @issue, current_user)
+    end
+
+    def referenced_issues
+      @referenced_issues ||= begin
+        issue_references = params[:issue_references]
+        text = issue_references.join(' ')
+
+        extractor = Gitlab::ReferenceExtractor.new(@issue.project, @current_user)
+        extractor.analyze(text)
+
+        extractor.issues.select do |issue|
+          can?(current_user, :admin_issue_link, issue)
+        end
+      end
+    end
+  end
+end

app/services/issue_links/destroy_service.rb

+module IssueLinks
+  class DestroyService < BaseService
+    def initialize(issue_link, user)
+      @issue_link = issue_link
+      @current_user = user
+      @issue = issue_link.source
+      @referenced_issue = issue_link.target
+    end
+
+    def execute
+      remove_relation
+      create_notes
+
+      success(message: 'Relation was removed')
+    end
+
+    private
+
+    def remove_relation
+      @issue_link.destroy!
+    end
+
+    def create_notes
+      SystemNoteService.unrelate_issue(@issue, @referenced_issue, current_user)
+      SystemNoteService.unrelate_issue(@referenced_issue, @issue, current_user)
+    end
+  end
+end

app/services/issue_links/list_service.rb

+module IssueLinks
+  class ListService
+    include Gitlab::Routing
+
+    def initialize(issue, user)
+      @issue, @current_user, @project = issue, user, issue.project
+    end
+
+    def execute
+      issues.map do |referenced_issue|
+        {
+          id: referenced_issue.id,
+          iid: referenced_issue.iid,
+          title: referenced_issue.title,
+          state: referenced_issue.state,
+          project_path: referenced_issue.project.path,
+          namespace_full_path: referenced_issue.project.namespace.full_path,
+          path: namespace_project_issue_path(referenced_issue.project.namespace, referenced_issue.project, referenced_issue.iid),
+          destroy_relation_path: destroy_relation_path(referenced_issue)
+        }
+      end
+    end
+
+    private
+
+    def issues
+      related_issues = Issue
+                         .select(['issues.*', 'issue_links.id AS issue_links_id'])
+                         .joins("INNER JOIN issue_links ON
+                                 (issue_links.source_id = issues.id AND issue_links.target_id = #{@issue.id})
+                                 OR
+                                 (issue_links.target_id = issues.id AND issue_links.source_id = #{@issue.id})")
+                         .preload(project: :namespace)
+                         .reorder('issue_links_id')
+
+      Ability.issues_readable_by_user(related_issues, @current_user)
+    end
+
+    def destroy_relation_path(issue)
+      # Make sure the user can admin both the current issue AND the
+      # referenced issue projects in order to return the removal link.
+      if can_destroy_issue_link_on_current_project? && can_destroy_issue_link?(issue.project)
+        namespace_project_issue_link_path(@project.namespace,
+                                          @issue.project,
+                                          @issue.iid,
+                                          issue.issue_links_id)
+      end
+    end
+
+    def can_destroy_issue_link_on_current_project?
+      return @can_destroy_on_current_project if defined?(@can_destroy_on_current_project)
+
+      @can_destroy_on_current_project = can_destroy_issue_link?(@project)
+    end
+
+    def can_destroy_issue_link?(project)
+      Ability.allowed?(@current_user, :admin_issue_link, project)
+    end
+  end
+end

app/services/system_note_service.rb

@@ -552,6 +552,38 @@ def noteable_moved(noteable, project, noteable_ref, author, direction:)
     create_note(NoteSummary.new(noteable, project, author, body, action: 'moved'))
   end
+  #
+  # noteable     - Noteable object
+  # noteable_ref - Referenced noteable object
+  # user         - User performing reference
+  #
+  # Example Note text:
+  #
+  #   "marked this issue as related to gitlab-ce#9001"
+  #
+  # Returns the created Note object
+  def relate_issue(noteable, noteable_ref, user)
+    body = "marked this issue as related to #{noteable_ref.to_reference(noteable.project)}"
+
+    create_note(NoteSummary.new(noteable, noteable.project, user, body, action: 'relate'))
+  end
+
+  #
+  # noteable     - Noteable object
+  # noteable_ref - Referenced noteable object
+  # user         - User performing reference
+  #
+  # Example Note text:
+  #
+  #   "removed the relation with gitlab-ce#9001"
+  #
+  # Returns the created Note object
+  def unrelate_issue(noteable, noteable_ref, user)
+    body = "removed the relation with #{noteable_ref.to_reference(noteable.project)}"
+
+    create_note(NoteSummary.new(noteable, noteable.project, user, body, action: 'unrelate'))
+  end
+
   # Called when the merge request is approved by user
   #
   # noteable - Noteable object

changelogs/unreleased-ee/2001-related-issues.yml

+---
+title: Allows manually adding bi-directional relationships between issues in the issue page (EES feature)
+merge_request:
+author:

config/locales/en.yml

@@ -3,6 +3,11 @@
 en:
   hello: "Hello world"
+  activerecord:
+    attributes:
+      issue_link:
+        source: Source issue
+        target: Target issue
   errors:
     messages:
       label_already_exists_at_group_level: "already exists at group level for %{group}. Please choose another one."

config/routes/project.rb

@@ -311,6 +311,8 @@
           post :bulk_update
           post :export_csv
         end
+
+        resources :issue_links, only: [:index, :create, :destroy], as: 'links', path: 'links'
       end
       resources :project_members, except: [:show, :new, :edit], constraints: { id: /[a-zA-Z.\/0-9_\-#%+]+/ }, concerns: :access_requestable do

db/migrate/20170517162048_create_issue_links_table.rb

+class CreateIssueLinksTable < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    create_table :issue_links do |t|
+      t.integer :source_id, null: false, index: true
+      t.integer :target_id, null: false, index: true
+
+      t.timestamps null: true
+    end
+
+    add_index :issue_links, [:source_id, :target_id], unique: true
+
+    add_concurrent_foreign_key :issue_links, :issues, column: :source_id
+    add_concurrent_foreign_key :issue_links, :issues, column: :target_id
+  end
+
+  def down
+    drop_table :issue_links
+  end
+end

db/schema.rb

@@ -591,6 +591,17 @@
   add_index "issue_assignees", ["issue_id", "user_id"], name: "index_issue_assignees_on_issue_id_and_user_id", unique: true, using: :btree
   add_index "issue_assignees", ["user_id"], name: "index_issue_assignees_on_user_id", using: :btree
+  create_table "issue_links", force: :cascade do |t|
+    t.integer "source_id", null: false
+    t.integer "target_id", null: false
+    t.datetime "created_at"
+    t.datetime "updated_at"
+  end
+
+  add_index "issue_links", ["source_id", "target_id"], name: "index_issue_links_on_source_id_and_target_id", unique: true, using: :btree
+  add_index "issue_links", ["source_id"], name: "index_issue_links_on_source_id", using: :btree
+  add_index "issue_links", ["target_id"], name: "index_issue_links_on_target_id", using: :btree
+
   create_table "issue_metrics", force: :cascade do |t|
     t.integer "issue_id", null: false
     t.datetime "first_mentioned_in_commit_at"
@@ -1708,6 +1719,8 @@
   add_foreign_key "container_repositories", "projects"
   add_foreign_key "issue_assignees", "issues", name: "fk_b7d881734a", on_delete: :cascade
   add_foreign_key "issue_assignees", "users", name: "fk_5e0c8d9154", on_delete: :cascade
+  add_foreign_key "issue_links", "issues", column: "source_id", name: "fk_c900194ff2", on_delete: :cascade
+  add_foreign_key "issue_links", "issues", column: "target_id", name: "fk_e71bb44f1f", on_delete: :cascade
   add_foreign_key "issue_metrics", "issues", on_delete: :cascade
   add_foreign_key "label_priorities", "labels", on_delete: :cascade
   add_foreign_key "label_priorities", "projects", on_delete: :cascade

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -284,13 +284,18 @@ def go(extra_params = {})
       context 'number of queries' do
         it 'verifies number of queries' do
+          RequestStore.begin!
+
           # pre-create objects
           merge_request
           recorded = ActiveRecord::QueryRecorder.new { go(format: :json) }
-          expect(recorded.count).to be_within(10).of(100)
+          expect(recorded.count).to be_within(1).of(31)
           expect(recorded.cached_count).to eq(0)
+
+          RequestStore.end!
+          RequestStore.clear!
         end
       end
     end

spec/factories/issue_links.rb

+FactoryGirl.define do
+  factory :issue_link do
+    source factory: :issue
+    target factory: :issue
+  end
+end

spec/finders/issues_finder_spec.rb

@@ -288,6 +288,18 @@
         expect(issues.count).to eq 0
       end
+
+      it 'returns disabled issues if feature_availability_check param set to false' do
+        [project1, project2].each do |project|
+          project.project_feature.update!(issues_access_level: ProjectFeature::DISABLED)
+        end
+
+        issues = described_class
+                   .new(search_user, params.reverse_merge(scope: scope, state: 'opened', feature_availability_check: false))
+                   .execute
+
+        expect(issues.count).to eq 3
+      end
     end
   end

spec/models/issue_link_spec.rb

+require 'spec_helper'
+
+describe IssueLink do
+  describe 'Associations' do
+    it { is_expected.to belong_to(:source).class_name('Issue') }
+    it { is_expected.to belong_to(:target).class_name('Issue') }
+  end
+
+  describe 'Validation' do
+    subject { create :issue_link }
+
+    it { is_expected.to validate_presence_of(:source) }
+    it { is_expected.to validate_presence_of(:target) }
+    it do
+      is_expected.to validate_uniqueness_of(:source)
+        .scoped_to(:target_id)
+        .with_message(/already related/)
+    end
+
+    context 'self relation' do
+      let(:issue) { create :issue }
+
+      context 'cannot be validated' do
+        it 'does not invalidate object with self relation error' do
+          issue_link = build :issue_link, source: issue, target: nil
+
+          issue_link.valid?
+
+          expect(issue_link.errors[:source]).to be_empty
+        end
+      end
+
+      context 'can be invalidated' do
+        it 'invalidates object' do
+          issue_link = build :issue_link, source: issue, target: issue
+
+          expect(issue_link).to be_invalid
+          expect(issue_link.errors[:source]).to include('cannot be related to itself')
+        end
+      end
+    end
+  end
+end