Merge branch '335722-set-work-item-type-on-issue-creation' into 'master'

Set WorkItem type on issue creation/update

See merge request gitlab-org/gitlab!67638

app/models/work_item/type.rb

@@ -34,6 +34,14 @@ class WorkItem::Type < ApplicationRecord
   validates :name, length: { maximum: 255 }
   validates :icon_name, length: { maximum: 255 }
 
+  def self.default_by_type(type)
+    find_by(namespace_id: nil, base_type: type)
+  end
+
+  def self.default_issue_type
+    default_by_type(:issue)
+  end
+
   private
 
   def strip_whitespace

app/services/issues/base_service.rb

@@ -34,6 +34,13 @@ module Issues
 
     private
 
+    def find_work_item_type_id(issue_type)
+      work_item_type = WorkItem::Type.default_by_type(issue_type)
+      work_item_type ||= WorkItem::Type.default_issue_type
+
+      work_item_type.id
+    end
+
     def filter_params(issue)
       super
 

app/services/issues/build_service.rb

@@ -6,6 +6,7 @@ module Issues
 
     def execute
       filter_resolve_discussion_params
+
       @issue = project.issues.new(issue_params).tap do |issue|
         ensure_milestone_available(issue)
       end
@@ -60,6 +61,13 @@ module Issues
 
     def issue_params
       @issue_params ||= build_issue_params
+
+      # If :issue_type is nil then params[:issue_type] was either nil
+      # or not permitted.  Either way, the :issue_type will default
+      # to the column default of `issue`. And that means we need to
+      # ensure the work_item_type_id is set
+      @issue_params[:work_item_type_id] = get_work_item_type_id(@issue_params[:issue_type])
+      @issue_params
     end
 
     private
@@ -81,6 +89,11 @@ module Issues
       { author: current_user }
         .merge(issue_params_with_info_from_discussions)
         .merge(allowed_issue_params)
+        .with_indifferent_access
+    end
+
+    def get_work_item_type_id(issue_type = :issue)
+      find_work_item_type_id(issue_type)
     end
   end
 end

app/services/issues/update_service.rb

@@ -26,6 +26,8 @@ module Issues
     end
 
     def before_update(issue, skip_spam_check: false)
+      change_work_item_type(issue)
+
       return if skip_spam_check
 
       Spam::SpamActionService.new(
@@ -36,6 +38,14 @@ module Issues
       ).execute
     end
 
+    def change_work_item_type(issue)
+      return unless issue.changed_attributes['issue_type']
+
+      type_id = find_work_item_type_id(issue.issue_type)
+
+      issue.work_item_type_id = type_id
+    end
+
     def handle_changes(issue, options)
       super
       old_associations = options.fetch(:old_associations, {})

ee/app/services/ee/issues/build_service.rb

@@ -17,7 +17,7 @@ module EE
       # The template params are filled in here, and might be overwritten by super
       override :build_issue_params
       def build_issue_params
-        issue_params_from_template.merge(super)
+        issue_params_from_template.merge(super).with_indifferent_access
       end
     end
   end

ee/spec/requests/api/issues_spec.rb

@@ -475,8 +475,10 @@ RSpec.describe API::Issues, :mailer do
 
   describe "POST /projects/:id/issues" do
     it 'creates a new project issue' do
-      post api("/projects/#{project.id}/issues", user),
-        params: { title: 'new issue', labels: 'label, label2', weight: 101, assignee_ids: [user2.id] }
+      expect do
+        post api("/projects/#{project.id}/issues", user),
+          params: { title: 'new issue', labels: 'label, label2', weight: 101, assignee_ids: [user2.id] }
+      end.to change(Issue, :count).by(1)
 
       expect(response).to have_gitlab_http_status(:created)
       expect(json_response['title']).to eq('new issue')
@@ -486,6 +488,7 @@ RSpec.describe API::Issues, :mailer do
       expect(json_response['weight']).to eq(101)
       expect(json_response['assignee']['name']).to eq(user2.name)
       expect(json_response['assignees'].first['name']).to eq(user2.name)
+      expect(Issue.last.work_item_type.base_type).to eq('issue')
     end
 
     it_behaves_like 'with epic parameter' do

spec/controllers/boards/issues_controller_spec.rb

@@ -428,17 +428,21 @@ RSpec.describe Boards::IssuesController do
 
   describe 'POST create' do
     context 'with valid params' do
-      it 'returns a successful 200 response' do
+      before do
         create_issue user: user, board: board, list: list1, title: 'New issue'
+      end
 
+      it 'returns a successful 200 response' do
         expect(response).to have_gitlab_http_status(:ok)
       end
 
       it 'returns the created issue' do
-        create_issue user: user, board: board, list: list1, title: 'New issue'
-
         expect(response).to match_response_schema('entities/issue_board')
       end
+
+      it 'sets the default work_item_type' do
+        expect(Issue.last.work_item_type.base_type).to eq('issue')
+      end
     end
 
     context 'with invalid params' do

spec/controllers/projects/issues_controller_spec.rb

@@ -1176,12 +1176,22 @@ RSpec.describe Projects::IssuesController do
       project.issues.first
     end
 
+    context 'when creating an incident' do
+      it 'sets the correct issue_type' do
+        issue = post_new_issue(issue_type: 'incident')
+
+        expect(issue.issue_type).to eq('incident')
+        expect(issue.work_item_type.base_type).to eq('incident')
+      end
+    end
+
     it 'creates the issue successfully', :aggregate_failures do
       issue = post_new_issue
 
       expect(issue).to be_a(Issue)
       expect(issue.persisted?).to eq(true)
       expect(issue.issue_type).to eq('issue')
+      expect(issue.work_item_type.base_type).to eq('issue')
     end
 
     context 'resolving discussions in MergeRequest' do

spec/factories/issues.rb

@@ -8,6 +8,7 @@ FactoryBot.define do
     updated_by { author }
     relative_position { RelativePositioning::START_POSITION }
     issue_type { :issue }
+    association :work_item_type, :default
 
     trait :confidential do
       confidential { true }
@@ -59,6 +60,7 @@ FactoryBot.define do
 
     factory :incident do
       issue_type { :incident }
+      association :work_item_type, :default, :incident
     end
   end
 end

spec/factories/work_item/work_item_types.rb

@@ -8,6 +8,17 @@ FactoryBot.define do
     base_type { WorkItem::Type.base_types[:issue] }
     icon_name { 'issue-type-issue' }
 
+    initialize_with do
+      type_base_attributes = attributes.with_indifferent_access.slice(:base_type, :namespace, :namespace_id)
+
+      # Expect base_types to exist on the DB
+      if type_base_attributes.slice(:namespace, :namespace_id).compact.empty?
+        WorkItem::Type.find_or_initialize_by(type_base_attributes).tap { |type| type.assign_attributes(attributes) }
+      else
+        WorkItem::Type.new(attributes)
+      end
+    end
+
     trait :default do
       namespace { nil }
     end

spec/migrations/20210804150320_create_base_work_item_types_spec.rb

@@ -6,7 +6,17 @@ require_migration!('create_base_work_item_types')
 RSpec.describe CreateBaseWorkItemTypes, :migration do
   let!(:work_item_types) { table(:work_item_types) }
 
+  after(:all) do
+    # Make sure base types are recreated after running the migration
+    # because migration specs are not run in a transaction
+    WorkItem::Type.delete_all
+    Gitlab::DatabaseImporters::WorkItems::BaseTypeImporter.import
+  end
+
   it 'creates default data' do
+    # Need to delete all as base types are seeded before entire test suite
+    WorkItem::Type.delete_all
+
     reversible_migration do |migration|
       migration.before -> {
         # Depending on whether the migration has been run before,

spec/migrations/20210831203408_upsert_base_work_item_types_spec.rb

@@ -6,8 +6,18 @@ require_migration!('upsert_base_work_item_types')
 RSpec.describe UpsertBaseWorkItemTypes, :migration do
   let!(:work_item_types) { table(:work_item_types) }
 
+  after(:all) do
+    # Make sure base types are recreated after running the migration
+    # because migration specs are not run in a transaction
+    WorkItem::Type.delete_all
+    Gitlab::DatabaseImporters::WorkItems::BaseTypeImporter.import
+  end
+
   context 'when no default types exist' do
     it 'creates default data' do
+      # Need to delete all as base types are seeded before entire test suite
+      WorkItem::Type.delete_all
+
       expect(work_item_types.count).to eq(0)
 
       reversible_migration do |migration|
@@ -26,10 +36,6 @@ RSpec.describe UpsertBaseWorkItemTypes, :migration do
   end
 
   context 'when default types already exist' do
-    before do
-      Gitlab::DatabaseImporters::WorkItems::BaseTypeImporter.import
-    end
-
     it 'does not create default types again' do
       expect(work_item_types.all.pluck(:base_type)).to match_array(WorkItem::Type.base_types.values)
 

spec/models/work_item/type_spec.rb

@@ -19,8 +19,10 @@ RSpec.describe WorkItem::Type do
       it 'deletes type but not unrelated issues' do
         type = create(:work_item_type)
 
+        expect(WorkItem::Type.count).to eq(5)
+
         expect { type.destroy! }.not_to change(Issue, :count)
-        expect(WorkItem::Type.count).to eq 0
+        expect(WorkItem::Type.count).to eq(4)
       end
     end
 
@@ -28,7 +30,7 @@ RSpec.describe WorkItem::Type do
       type = create(:work_item_type, work_items: [work_item])
 
       expect { type.destroy! }.to raise_error(ActiveRecord::InvalidForeignKey)
-      expect(Issue.count).to eq 1
+      expect(Issue.count).to eq(1)
     end
   end
 

spec/rake_helper.rb

@@ -12,6 +12,6 @@ RSpec.configure do |config|
   end
 
   config.after(:all) do
-    delete_from_all_tables!
+    delete_from_all_tables!(except: deletion_except_tables)
   end
 end

spec/requests/api/graphql/mutations/issues/create_spec.rb

@@ -39,11 +39,14 @@ RSpec.describe 'Create an issue' do
     end
 
     it 'creates the issue' do
-      post_graphql_mutation(mutation, current_user: current_user)
+      expect do
+        post_graphql_mutation(mutation, current_user: current_user)
+      end.to change(Issue, :count).by(1)
 
       expect(response).to have_gitlab_http_status(:success)
       expect(mutation_response['issue']).to include(input)
       expect(mutation_response['issue']).to include('discussionLocked' => true)
+      expect(Issue.last.work_item_type.base_type).to eq('issue')
     end
   end
 end

spec/requests/api/graphql/mutations/issues/update_spec.rb

@@ -44,6 +44,19 @@ RSpec.describe 'Update of an existing issue' do
       expect(mutation_response['issue']).to include('discussionLocked' => true)
     end
 
+    context 'when issue_type is updated' do
+      let(:input) { { 'iid' => issue.iid.to_s, 'type' => 'INCIDENT' } }
+
+      it 'updates issue_type and work_item_type' do
+        expect do
+          post_graphql_mutation(mutation, current_user: current_user)
+          issue.reload
+        end.to change { issue.work_item_type.base_type }.from('issue').to('incident').and(
+          change(issue, :issue_type).from('issue').to('incident')
+        )
+      end
+    end
+
     context 'setting labels' do
       let(:mutation) do
         graphql_mutation(:update_issue, input_params) do

spec/services/issues/build_service_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Issues::BuildService do
+  using RSpec::Parameterized::TableSyntax
+
   let_it_be(:project) { create(:project, :repository) }
   let_it_be(:developer) { create(:user) }
   let_it_be(:guest) { create(:user) }
@@ -144,6 +146,8 @@ RSpec.describe Issues::BuildService do
         issue = build_issue(milestone_id: milestone.id)
 
         expect(issue.milestone).to eq(milestone)
+        expect(issue.issue_type).to eq('issue')
+        expect(issue.work_item_type.base_type).to eq('issue')
       end
 
       it 'sets milestone to nil if it is not available for the project' do
@@ -152,6 +156,15 @@ RSpec.describe Issues::BuildService do
 
         expect(issue.milestone).to be_nil
       end
+
+      context 'when issue_type is incident' do
+        it 'sets the correct issue type' do
+          issue = build_issue(issue_type: 'incident')
+
+          expect(issue.issue_type).to eq('incident')
+          expect(issue.work_item_type.base_type).to eq('incident')
+        end
+      end
     end
 
     context 'as guest' do
@@ -165,22 +178,13 @@ RSpec.describe Issues::BuildService do
       end
 
       context 'setting issue type' do
-        it 'defaults to issue if issue_type not given' do
-          issue = build_issue
+        shared_examples 'builds an issue' do
+          specify do
+            issue = build_issue(issue_type: issue_type)
 
-          expect(issue).to be_issue
-        end
-
-        it 'sets issue' do
-          issue = build_issue(issue_type: 'issue')
-
-          expect(issue).to be_issue
-        end
-
-        it 'sets incident' do
-          issue = build_issue(issue_type: 'incident')
-
-          expect(issue).to be_incident
+            expect(issue.issue_type).to eq(resulting_issue_type)
+            expect(issue.work_item_type_id).to eq(work_item_type_id)
+          end
         end
 
         it 'cannot set invalid issue type' do
@@ -188,6 +192,24 @@ RSpec.describe Issues::BuildService do
 
           expect(issue).to be_issue
         end
+
+        context 'with a corresponding WorkItem::Type' do
+          let_it_be(:type_issue_id) { WorkItem::Type.default_issue_type.id }
+          let_it_be(:type_incident_id) { WorkItem::Type.default_by_type(:incident).id }
+
+          where(:issue_type, :work_item_type_id, :resulting_issue_type) do
+            nil           | ref(:type_issue_id)       | 'issue'
+            'issue'       | ref(:type_issue_id)       | 'issue'
+            'incident'    | ref(:type_incident_id)    | 'incident'
+            'test_case'   | ref(:type_issue_id)       | 'issue' # update once support for test_case is enabled
+            'requirement' | ref(:type_issue_id)       | 'issue' # update once support for requirement is enabled
+            'invalid'     | ref(:type_issue_id)       | 'issue'
+          end
+
+          with_them do
+            it_behaves_like 'builds an issue'
+          end
+        end
       end
     end
   end

spec/services/issues/create_service_spec.rb

@@ -43,10 +43,11 @@ RSpec.describe Issues::CreateService do
 
         expect(issue).to be_persisted
         expect(issue.title).to eq('Awesome issue')
-        expect(issue.assignees).to eq [assignee]
-        expect(issue.labels).to match_array labels
-        expect(issue.milestone).to eq milestone
-        expect(issue.due_date).to eq Date.tomorrow
+        expect(issue.assignees).to eq([assignee])
+        expect(issue.labels).to match_array(labels)
+        expect(issue.milestone).to eq(milestone)
+        expect(issue.due_date).to eq(Date.tomorrow)
+        expect(issue.work_item_type.base_type).to eq('issue')
       end
 
       context 'when skip_system_notes is true' do

spec/services/issues/update_service_spec.rb

@@ -228,15 +228,19 @@ RSpec.describe Issues::UpdateService, :mailer do
         context 'from incident to issue' do
           let(:issue) { create(:incident, project: project) }
 
+          it 'changed from an incident to an issue type' do
+            expect { update_issue(issue_type: 'issue') }
+              .to change(issue, :issue_type).from('incident').to('issue')
+              .and(change { issue.work_item_type.base_type }.from('incident').to('issue'))
+          end
+
           context 'for an incident with multiple labels' do
             let(:issue) { create(:incident, project: project, labels: [label_1, label_2]) }
 
-            before do
-              update_issue(issue_type: 'issue')
-            end
-
             it 'removes an `incident` label if one exists on the incident' do
-              expect(issue.labels).to eq([label_2])
+              expect { update_issue(issue_type: 'issue') }.to change(issue, :label_ids)
+                .from(containing_exactly(label_1.id, label_2.id))
+                .to([label_2.id])
             end
           end
 
@@ -244,12 +248,10 @@ RSpec.describe Issues::UpdateService, :mailer do
             let(:issue) { create(:incident, project: project, labels: [label_1, label_2]) }
             let(:params) { { label_ids: [label_1.id, label_2.id], remove_label_ids: [] } }
 
-            before do
-              update_issue(issue_type: 'issue')
-            end
-
             it 'adds an incident label id to remove_label_ids for it to be removed' do
-              expect(issue.label_ids).to contain_exactly(label_2.id)
+              expect { update_issue(issue_type: 'issue') }.to change(issue, :label_ids)
+                .from(containing_exactly(label_1.id, label_2.id))
+                .to([label_2.id])
             end
           end
         end

spec/spec_helper.rb

@@ -220,6 +220,8 @@ RSpec.configure do |config|
     # Enable all features by default for testing
     # Reset any changes in after hook.
     stub_all_feature_flags
+
+    TestEnv.seed_db
   end
 
   config.after(:all) do

spec/support/database_cleaner.rb

@@ -14,7 +14,7 @@ RSpec.configure do |config|
   end
 
   config.append_after(:context, :migration) do
-    delete_from_all_tables!
+    delete_from_all_tables!(except: ['work_item_types'])
 
     # Postgres maximum number of columns in a table is 1600 (https://github.com/postgres/postgres/blob/de41869b64d57160f58852eab20a27f248188135/src/include/access/htup_details.h#L23-L47).
     # And since:
@@ -61,7 +61,7 @@ RSpec.configure do |config|
 
     example.run
 
-    delete_from_all_tables!
+    delete_from_all_tables!(except: ['work_item_types'])
 
     self.class.use_transactional_tests = true
   end

spec/support/db_cleaner.rb

@@ -12,7 +12,7 @@ module DbCleaner
   end
 
   def deletion_except_tables
-    []
+    ['work_item_types']
   end
 
   def setup_database_cleaner

spec/support/helpers/test_env.rb

@@ -452,6 +452,10 @@ module TestEnv
     example_group
   end
 
+  def seed_db
+    Gitlab::DatabaseImporters::WorkItems::BaseTypeImporter.import
+  end
+
   private
 
   # These are directories that should be preserved at cleanup time

spec/support/shared_examples/services/incident_shared_examples.rb

@@ -13,6 +13,7 @@
 RSpec.shared_examples 'incident issue' do
   it 'has incident as issue type' do
     expect(issue.issue_type).to eq('incident')
+    expect(issue.work_item_type.base_type).to eq('incident')
   end
 end
 
@@ -41,6 +42,7 @@ RSpec.shared_examples 'not an incident issue' do
 
   it 'has not incident as issue type' do
     expect(issue.issue_type).not_to eq('incident')
+    expect(issue.work_item_type.base_type).not_to eq('incident')
   end
 
   it 'has not an incident label' do

spec/support/shared_examples/work_item_base_types_importer.rb

@@ -2,6 +2,9 @@
 
 RSpec.shared_examples 'work item base types importer' do
   it 'creates all base work item types' do
+    # Fixtures need to run on a pristine DB, but the test suite preloads the base types before(:suite)
+    WorkItem::Type.delete_all
+
     expect { subject }.to change(WorkItem::Type, :count).from(0).to(WorkItem::Type::BASE_TYPES.count)
   end
 end