Merge branch '8333-prepare-refactoring' into 'master'

Refactor issuables links to be more general

See merge request gitlab-org/gitlab-ee!8692

ee/app/assets/javascripts/related_issues/components/related_issues_root.vue

@@ -114,7 +114,7 @@ export default {
         RelatedIssuesService.remove(issueToRemove.relation_path)
           .then(res => res.json())
           .then(data => {
-            this.store.setRelatedIssues(data.issues);
+            this.store.setRelatedIssues(data.issuables);
           })
           .catch(res => {
             if (res && res.status !== 404) {
@@ -142,7 +142,7 @@ export default {
           .then(data => {
             // We could potentially lose some pending issues in the interim here
             this.store.setPendingReferences([]);
-            this.store.setRelatedIssues(data.issues);
+            this.store.setRelatedIssues(data.issuables);
 
             this.isSubmitting = false;
             // Close the form on submission

ee/app/assets/javascripts/related_issues/services/related_issues_service.js

@@ -16,7 +16,7 @@ class RelatedIssuesService {
     return this.relatedIssuesResource.save(
       {},
       {
-        issue_references: newIssueReferences,
+        issuable_references: newIssueReferences,
       },
     );
   }

ee/app/controllers/concerns/epic_relations.rb

+# frozen_string_literal: true
+
+module EpicRelations
+  extend ActiveSupport::Concern
+  include IssuableLinks
+
+  included do
+    skip_before_action :authorize_destroy_issuable!
+    skip_before_action :authorize_create_epic!
+    skip_before_action :authorize_update_issuable!
+
+    before_action :authorize_admin_epic!, only: [:create, :destroy, :update]
+  end
+
+  def authorize_admin_epic!
+    render_403 unless can?(current_user, :admin_epic, epic)
+  end
+end

ee/app/controllers/concerns/issuable_links.rb

@@ -2,25 +2,33 @@
 
 module IssuableLinks
   def index
-    render json: issues
+    render json: issuables
   end
 
   def create
     result = create_service.execute
 
-    render json: { message: result[:message], issues: issues }, status: result[:http_status]
+    render json: { message: result[:message], issuables: issuables }, status: result[:http_status]
   end
 
   def destroy
     result = destroy_service.execute
 
-    render json: { issues: issues }, status: result[:http_status]
+    render json: { issuables: issuables }, status: result[:http_status]
   end
 
   private
 
+  def issuables
+    list_service.execute
+  end
+
+  def list_service
+    raise NotImplementedError
+  end
+
   def create_params
-    params.slice(:issue_references)
+    params.slice(:issuable_references)
   end
 
   def create_service

ee/app/controllers/groups/epic_issues_controller.rb

 # frozen_string_literal: true
 
 class Groups::EpicIssuesController < Groups::EpicsController
-  include IssuableLinks
+  include EpicRelations
 
-  skip_before_action :authorize_destroy_issuable!
-  skip_before_action :authorize_create_epic!
-  skip_before_action :authorize_update_issuable!
-
-  before_action :authorize_admin_epic!, only: [:create, :destroy, :update]
   before_action :authorize_issue_link_association!, only: [:destroy, :update]
 
   def update
@@ -26,12 +21,8 @@ class Groups::EpicIssuesController < Groups::EpicsController
     EpicIssues::DestroyService.new(link, current_user)
   end
 
-  def issues
-    EpicIssues::ListService.new(epic, current_user).execute
-  end
-
-  def authorize_admin_epic!
-    render_403 unless can?(current_user, :admin_epic, epic)
+  def list_service
+    EpicIssues::ListService.new(epic, current_user)
   end
 
   def authorize_issue_link_association!

ee/app/controllers/projects/issue_links_controller.rb

@@ -9,10 +9,6 @@ module Projects
 
     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
@@ -29,6 +25,10 @@ module Projects
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
+    def list_service
+      IssueLinks::ListService.new(issue, current_user)
+    end
+
     def create_service
       IssueLinks::CreateService.new(issue, current_user, create_params)
     end

ee/app/services/ee/issues/update_service.rb

@@ -25,7 +25,7 @@ module EE
         epic_param = params.delete(:epic)
 
         if epic_param
-          EpicIssues::CreateService.new(epic_param, current_user, { target_issue: issue }).execute
+          EpicIssues::CreateService.new(epic_param, current_user, { target_issuable: issue }).execute
         else
           link = EpicIssue.find_by(issue_id: issue.id) # rubocop: disable CodeReuse/ActiveRecord
 

ee/app/services/epic_issues/create_service.rb

@@ -3,7 +3,7 @@ module EpicIssues
     private
 
     # rubocop: disable CodeReuse/ActiveRecord
-    def relate_issues(referenced_issue)
+    def relate_issuables(referenced_issue)
       link = EpicIssue.find_or_initialize_by(issue: referenced_issue)
 
       affected_epics = [issuable]
@@ -41,17 +41,17 @@ module EpicIssues
       { group: issuable.group }
     end
 
-    def linkable_issues(issues)
+    def linkable_issuables(issues)
       @linkable_issues ||= begin
         return [] unless can?(current_user, :admin_epic, issuable.group)
 
         issues.select do |issue|
-          issuable_group_descendants.include?(issue.project.group) && !previous_related_issues.include?(issue)
+          issuable_group_descendants.include?(issue.project.group) && !previous_related_issuables.include?(issue)
         end
       end
     end
 
-    def previous_related_issues
+    def previous_related_issuables
       @related_issues ||= issuable.issues.to_a
     end
 

ee/app/services/epic_issues/list_service.rb

@@ -2,7 +2,7 @@ module EpicIssues
   class ListService < IssuableLinks::ListService
     private
 
-    def issues
+    def child_issuables
       return [] unless issuable&.group&.feature_available?(:epics)
 
       issuable.issues_readable_by(current_user)

ee/app/services/issuable_links/create_service.rb

@@ -11,58 +11,62 @@ module IssuableLinks
       # otherwise create issue links for the issues which
       # are still not assigned and return success message.
       if render_conflict_error?
-        return error('Issue(s) already assigned', 409)
+        return error(issuables_assigned_message, 409)
       end
 
       if render_not_found_error?
-        return error('No Issue found for given params', 404)
+        return error(issuables_not_found_message, 404)
       end
 
-      create_issue_links
+      create_links
       success
     end
 
     private
 
     def render_conflict_error?
-      referenced_issues.present? && (referenced_issues - previous_related_issues).empty?
+      referenced_issuables.present? && (referenced_issuables - previous_related_issuables).empty?
     end
 
     def render_not_found_error?
-      linkable_issues(referenced_issues).empty?
+      linkable_issuables(referenced_issuables).empty?
     end
 
-    def create_issue_links
-      issues = linkable_issues(referenced_issues)
+    def create_links
+      objects = linkable_issuables(referenced_issuables)
 
-      issues.each do |referenced_issue|
-        relate_issues(referenced_issue) do |params|
-          create_notes(referenced_issue, params)
+      objects.each do |referenced_object|
+        relate_issuables(referenced_object) do |params|
+          create_notes(referenced_object, params)
         end
       end
     end
 
-    def referenced_issues
-      @referenced_issues ||= begin
-        target_issue = params[:target_issue]
+    def referenced_issuables
+      @referenced_issuables ||= begin
+        target_issuable = params[:target_issuable]
 
-        if params[:issue_references].present?
-          extract_issues_from_references
-        elsif target_issue
-          [target_issue]
+        if params[:issuable_references].present?
+          extract_references
+        elsif target_issuable
+          [target_issuable]
         else
           []
         end
       end
     end
 
-    def extract_issues_from_references
-      issue_references = params[:issue_references]
-      text = issue_references.join(' ')
+    def extract_references
+      issuable_references = params[:issuable_references]
+      text = issuable_references.join(' ')
 
-      extractor = Gitlab::ReferenceExtractor.new(issuable.project, @current_user)
+      extractor = Gitlab::ReferenceExtractor.new(issuable.project, current_user)
       extractor.analyze(text, extractor_context)
 
+      references(extractor)
+    end
+
+    def references(extractor)
       extractor.issues
     end
 
@@ -70,16 +74,24 @@ module IssuableLinks
       {}
     end
 
-    def linkable_issues(issues)
+    def linkable_issuables(objects)
       raise NotImplementedError
     end
 
-    def previous_related_issues
+    def previous_related_issuables
       raise NotImplementedError
     end
 
-    def relate_issues(referenced_issue)
+    def relate_issuables(referenced_object)
       raise NotImplementedError
     end
+
+    def issuables_assigned_message
+      'Issue(s) already assigned'
+    end
+
+    def issuables_not_found_message
+      'No Issue found for given params'
+    end
   end
 end

ee/app/services/issuable_links/list_service.rb

@@ -9,29 +9,33 @@ module IssuableLinks
     end
 
     def execute
-      issues.map do |referenced_issue|
-        to_hash(referenced_issue)
+      child_issuables.map do |referenced_object|
+        to_hash(referenced_object)
       end
     end
 
     private
 
-    def relation_path(issue)
+    def relation_path(object)
       raise NotImplementedError
     end
 
-    def reference(issue)
-      issue.to_reference(issuable.project)
+    def reference(object)
+      object.to_reference(issuable.project)
     end
 
-    def to_hash(issue)
+    def issuable_path(object)
+      project_issue_path(object.project, object.iid)
+    end
+
+    def to_hash(object)
       {
-        id: issue.id,
-        title: issue.title,
-        state: issue.state,
-        reference: reference(issue),
-        path: project_issue_path(issue.project, issue.iid),
-        relation_path: relation_path(issue)
+        id: object.id,
+        title: object.title,
+        state: object.state,
+        reference: reference(object),
+        path: issuable_path(object),
+        relation_path: relation_path(object)
       }
     end
   end

ee/app/services/issue_links/create_service.rb

 module IssueLinks
   class CreateService < IssuableLinks::CreateService
-    def relate_issues(referenced_issue)
+    def relate_issuables(referenced_issue)
       link = IssueLink.create(source: issuable, target: referenced_issue)
 
       yield if link.persisted?
     end
 
-    def linkable_issues(issues)
-      @linkable_issues ||= begin
+    def linkable_issuables(issues)
+      @linkable_issuables ||= begin
         issues.select { |issue| can?(current_user, :admin_issue_link, issue) }
       end
     end
@@ -17,7 +17,7 @@ module IssueLinks
       SystemNoteService.relate_issue(referenced_issue, issuable, current_user)
     end
 
-    def previous_related_issues
+    def previous_related_issuables
       @related_issues ||= issuable.related_issues(current_user).to_a
     end
   end

ee/app/services/issue_links/list_service.rb

@@ -4,7 +4,7 @@ module IssueLinks
 
     private
 
-    def issues
+    def child_issuables
       issuable.related_issues(current_user, preload: { project: :namespace })
     end
 

ee/lib/api/epic_issues.rb

@@ -89,7 +89,7 @@ module API
 
         issue = Issue.find(params[:issue_id])
 
-        create_params = { target_issue: issue }
+        create_params = { target_issuable: issue }
 
         result = ::EpicIssues::CreateService.new(epic, current_user, create_params).execute
 

ee/lib/api/issue_links.rb

@@ -35,7 +35,7 @@ module API
         target_issue = find_project_issue(declared_params[:target_issue_iid],
                                           declared_params[:target_project_id])
 
-        create_params = { target_issue: target_issue }
+        create_params = { target_issuable: target_issue }
 
         result = ::IssueLinks::CreateService
                    .new(source_issue, current_user, create_params)

ee/spec/controllers/groups/epic_issues_controller_spec.rb

@@ -65,7 +65,7 @@ describe Groups::EpicIssuesController do
     subject do
       reference = [issue.to_reference(full: true)]
 
-      post :create, group_id: group, epic_id: epic.to_param, issue_references: reference
+      post :create, group_id: group, epic_id: epic.to_param, issuable_references: reference
     end
 
     it_behaves_like 'unlicensed epics action'
@@ -81,7 +81,7 @@ describe Groups::EpicIssuesController do
           list_service_response = EpicIssues::ListService.new(epic, user).execute
 
           expect(response).to have_gitlab_http_status(200)
-          expect(json_response).to eq('message' => nil, 'issues' => list_service_response.as_json)
+          expect(json_response).to eq('message' => nil, 'issuables' => list_service_response.as_json)
         end
 
         it 'creates a new EpicIssue record' do

ee/spec/javascripts/issuable/related_issues/components/related_issues_root_spec.js

@@ -163,7 +163,7 @@ describe('RelatedIssuesRoot', () => {
           next(
             request.respondWith(
               JSON.stringify({
-                issues: [issuable1],
+                issuables: [issuable1],
                 result: {
                   message: 'something was successfully related',
                   status: 'success',
@@ -196,7 +196,7 @@ describe('RelatedIssuesRoot', () => {
           next(
             request.respondWith(
               JSON.stringify({
-                issues: [issuable1, issuable2],
+                issuables: [issuable1, issuable2],
                 result: {
                   message: 'something was successfully related',
                   status: 'success',

ee/spec/requests/projects/issue_links_controller_spec.rb

@@ -38,26 +38,26 @@ describe Projects::IssueLinksController do
 
     context 'with success' do
       let(:user_role) { :developer }
-      let(:issue_references) { [issue_b.to_reference] }
+      let(:issuable_references) { [issue_b.to_reference] }
 
       it 'returns success JSON' do
-        post namespace_project_issue_links_path(issue_links_params(issue_references: issue_references))
+        post namespace_project_issue_links_path(issue_links_params(issuable_references: issuable_references))
 
         list_service_response = IssueLinks::ListService.new(issue, user).execute
 
         expect(response).to have_gitlab_http_status(200)
         expect(json_response).to eq('message' => nil,
-                                    'issues' => list_service_response.as_json)
+                                    'issuables' => list_service_response.as_json)
       end
     end
 
     context 'with failure' do
       context 'when unauthorized' do
         let(:user_role) { :guest }
-        let(:issue_references) { [issue_b.to_reference] }
+        let(:issuable_references) { [issue_b.to_reference] }
 
         it 'returns 403' do
-          post namespace_project_issue_links_path(issue_links_params(issue_references: issue_references))
+          post namespace_project_issue_links_path(issue_links_params(issuable_references: issuable_references))
 
           expect(response).to have_gitlab_http_status(403)
         end
@@ -65,15 +65,15 @@ describe Projects::IssueLinksController do
 
       context 'when failing service result' do
         let(:user_role) { :developer }
-        let(:issue_references) { ['#999'] }
+        let(:issuable_references) { ['#999'] }
 
         it 'returns failure JSON' do
-          post namespace_project_issue_links_path(issue_links_params(issue_references: issue_references))
+          post namespace_project_issue_links_path(issue_links_params(issuable_references: issuable_references))
 
           list_service_response = IssueLinks::ListService.new(issue, user).execute
 
           expect(response).to have_gitlab_http_status(404)
-          expect(json_response).to eq('message' => 'No Issue found for given params', 'issues' => list_service_response.as_json)
+          expect(json_response).to eq('message' => 'No Issue found for given params', 'issuables' => list_service_response.as_json)
         end
       end
     end
@@ -121,7 +121,7 @@ describe Projects::IssueLinksController do
 
         list_service_response = IssueLinks::ListService.new(issue, user).execute
 
-        expect(json_response).to eq('issues' => list_service_response.as_json)
+        expect(json_response).to eq('issuables' => list_service_response.as_json)
       end
     end
 

ee/spec/services/epic_issues/create_service_spec.rb

@@ -14,7 +14,7 @@ describe EpicIssues::CreateService do
     let!(:existing_link) { create(:epic_issue, epic: epic, issue: issue3) }
 
     def assign_issue(references)
-      params = { issue_references: references }
+      params = { issuable_references: references }
 
       described_class.new(epic, user, params).execute
     end
@@ -93,9 +93,9 @@ describe EpicIssues::CreateService do
         end
 
         context 'when the reference list is empty' do
-          it 'returns an error' do
-            expect(assign_issue([])).to eq(message: 'No Issue found for given params', status: :error, http_status: 404)
-          end
+          subject { assign_issue([]) }
+
+          include_examples 'returns an error'
 
           it 'does not create a system note' do
             expect { assign_issue([]) }.not_to change { Note.count }
@@ -124,7 +124,7 @@ describe EpicIssues::CreateService do
               allow(extractor).to receive(:analyze)
               allow(extractor).to receive(:issues).and_return([issue])
 
-              params = { issue_references: [valid_reference] }
+              params = { issuable_references: [valid_reference] }
               control_count = ActiveRecord::QueryRecorder.new { described_class.new(epic, user, params).execute }.count
 
               user = create(:user)
@@ -135,7 +135,7 @@ describe EpicIssues::CreateService do
               group.add_developer(user)
 
               allow(extractor).to receive(:issues).and_return(issues)
-              params = { issue_references: issues.map { |i| i.to_reference(full: true) } }
+              params = { issuable_references: issues.map { |i| i.to_reference(full: true) } }
 
               # threshold 24 because 6 queries are generated for each insert
               # (savepoint, find, exists, relative_position get, insert, release savepoint)
@@ -238,7 +238,7 @@ describe EpicIssues::CreateService do
         let(:another_epic) { create(:epic, group: group) }
 
         subject do
-          params = { issue_references: [valid_reference] }
+          params = { issuable_references: [valid_reference] }
 
           described_class.new(another_epic, user, params).execute
         end

ee/spec/services/issue_links/create_service_spec.rb

@@ -20,7 +20,7 @@ describe IssueLinks::CreateService do
 
     context 'when the reference list is empty' do
       let(:params) do
-        { issue_references: [] }
+        { issuable_references: [] }
       end
 
       it 'returns error' do
@@ -30,7 +30,7 @@ describe IssueLinks::CreateService do
 
     context 'when Issue not found' do
       let(:params) do
-        { issue_references: ['#999'] }
+        { issuable_references: ['#999'] }
       end
 
       it 'returns error' do
@@ -43,14 +43,14 @@ describe IssueLinks::CreateService do
     end
 
     context 'when user has no permission to target project Issue' do
-      let(:target_issue) { create :issue }
+      let(:target_issuable) { create :issue }
 
       let(:params) do
-        { issue_references: [target_issue.to_reference(project)] }
+        { issuable_references: [target_issuable.to_reference(project)] }
       end
 
       it 'returns error' do
-        target_issue.project.add_guest(user)
+        target_issuable.project.add_guest(user)
 
         is_expected.to eq(message: 'No Issue found for given params', status: :error, http_status: 404)
       end
@@ -62,7 +62,7 @@ describe IssueLinks::CreateService do
 
     context 'source and target are the same issue' do
       let(:params) do
-        { issue_references: [issue.to_reference] }
+        { issuable_references: [issue.to_reference] }
       end
 
       it 'does not create notes' do
@@ -85,7 +85,7 @@ describe IssueLinks::CreateService do
       let(:another_project_issue_ref) { another_project_issue.to_reference(project) }
 
       let(:params) do
-        { issue_references: [issue_a_ref, another_project_issue_ref] }
+        { issuable_references: [issue_a_ref, another_project_issue_ref] }
       end
 
       before do
@@ -129,7 +129,7 @@ describe IssueLinks::CreateService do
       end
 
       let(:params) do
-        { issue_references: [issue_b.to_reference, issue_a.to_reference] }
+        { issuable_references: [issue_b.to_reference, issue_a.to_reference] }
       end
 
       it 'returns success status' do