Merge branch 'nd/use-work-item-attributes' into 'master'

Use attributes for syncing work item to epic

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/153975



Merged-by: Eugenia Grieff <egrieff@gitlab.com>
Approved-by: Alexandru Croitor <acroitor@gitlab.com>
Approved-by: Eugenia Grieff <egrieff@gitlab.com>
Co-authored-by: Nicolas Dular <ndular@gitlab.com>

app/services/work_items/callbacks/base.rb

@@ -11,4 +11,3 @@ def raise_error(message)
     end
   end
 end
-WorkItems::Callbacks::Base.prepend_mod

app/services/work_items/callbacks/description.rb

@@ -3,8 +3,6 @@
 module WorkItems
   module Callbacks
     class Description < Base
-      include Gitlab::Utils::StrongMemoize
-
       def after_initialize
         params[:description] = nil if excluded_in_new_type?
 
@@ -19,9 +17,6 @@ def after_initialize
       def update_description?
         params.present? && params.key?(:description) && has_permission?(:update_work_item)
       end
-      strong_memoize_attr :update_description?
     end
   end
 end
-
-WorkItems::Callbacks::Description.prepend_mod

app/services/work_items/callbacks/start_and_due_date.rb

@@ -3,8 +3,6 @@
 module WorkItems
   module Callbacks
     class StartAndDueDate < Base
-      include Gitlab::Utils::StrongMemoize
-
       def before_update
         return work_item.assign_attributes({ start_date: nil, due_date: nil }) if excluded_in_new_type?
 
@@ -18,9 +16,6 @@ def before_update
       def update_start_and_due_date?
         params.present? && has_permission?(:set_work_item_metadata)
       end
-      strong_memoize_attr :update_start_and_due_date?
     end
   end
 end
-
-WorkItems::Callbacks::StartAndDueDate.prepend_mod

ee/app/services/concerns/work_items/sync_as_epic.rb

@@ -6,6 +6,12 @@ module SyncAsEpic
 
     private
 
+    BASE_ATTRIBUTE_PARAMS = %i[
+      iid author_id created_at updated_at title title_html description description_html
+      confidential state_id last_edited_by_id last_edited_at external_key updated_by_id
+      closed_at closed_by_id imported_from
+    ].freeze
+
     def create_epic_for!(work_item)
       return true unless work_item.namespace.work_item_sync_to_epic_enabled?
 
@@ -22,7 +28,8 @@ def update_epic_for!(work_item)
       return true unless epic
       return true unless epic.group.work_item_sync_to_epic_enabled?
 
-      epic.update!(update_params(work_item))
+      epic.assign_attributes(update_params(work_item))
+      epic.save!(touch: false)
     rescue StandardError => error
       handle_error!(:update, error, work_item)
     end
@@ -30,11 +37,9 @@ def update_epic_for!(work_item)
     def create_params(work_item)
       epic_params = {}
 
-      epic_params[:author] = work_item.author
       epic_params[:group] = work_item.namespace
       epic_params[:issue_id] = work_item.id
       epic_params[:iid] = work_item.iid
-      epic_params[:created_at] = work_item.created_at
 
       parent_link = WorkItems::ParentLink.find_by_work_item_id(work_item.id)
 
@@ -44,42 +49,33 @@ def create_params(work_item)
       end
 
       epic_params
-        .merge(callback_params)
         .merge(base_attributes_params(work_item))
+        .merge(color_params(work_item))
+        .merge(date_params(work_item))
     end
 
     def update_params(work_item)
-      callback_params
+      {}
         .merge(base_attributes_params(work_item))
+        .merge(color_params(work_item))
+        .merge(date_params(work_item))
     end
 
     def base_attributes_params(work_item)
-      base_params = {}
-
-      if params.has_key?(:title)
-        base_params[:title] = params[:title]
-        base_params[:title_html] = work_item.title_html
-      end
-
-      base_params[:confidential] = params[:confidential] if params.has_key?(:confidential)
-      base_params[:updated_by] = work_item.updated_by
-      base_params[:updated_at] = work_item.updated_at
-      base_params[:external_key] = params[:external_key] if params[:external_key]
+      BASE_ATTRIBUTE_PARAMS.index_with { |attr| work_item[attr] }
+    end
 
-      if work_item.edited?
-        base_params[:last_edited_at] = work_item.last_edited_at
-        base_params[:last_edited_by] = work_item.last_edited_by
-      end
+    def color_params(work_item)
+      return {} unless work_item.color
 
-      base_params
+      { color: work_item.color.color }
     end
 
-    def callback_params
-      callbacks.reduce({}) do |params, callback|
-        next params unless callback.synced_epic_params.present?
-
-        params.merge!(callback.synced_epic_params)
-      end
+    def date_params(work_item)
+      {
+        start_date: work_item.start_date,
+        due_date: work_item.due_date
+      }
     end
 
     def handle_error!(action, error, work_item)

ee/app/services/ee/work_items/callbacks/base.rb

-# frozen_string_literal: true
-
-module EE
-  module WorkItems
-    module Callbacks
-      module Base
-        extend ::Gitlab::Utils::Override
-
-        def synced_epic_params
-          @synced_epic_params ||= {}
-        end
-      end
-    end
-  end
-end

ee/app/services/ee/work_items/callbacks/description.rb

-# frozen_string_literal: true
-
-module EE
-  module WorkItems
-    module Callbacks
-      module Description
-        extend ::Gitlab::Utils::Override
-
-        override :after_initialize
-        def after_initialize
-          super
-
-          return unless update_description?
-
-          synced_epic_params[:description] = params[:description]
-          synced_epic_params[:description_html] = work_item.description_html
-        end
-      end
-    end
-  end
-end

ee/app/services/ee/work_items/callbacks/start_and_due_date.rb

-# frozen_string_literal: true
-
-module EE
-  module WorkItems
-    module Callbacks
-      module StartAndDueDate
-        extend ::Gitlab::Utils::Override
-
-        override :before_update
-        def before_update
-          super
-          return unless update_start_and_due_date?
-
-          synced_epic_params[:start_date] = params[:start_date]
-          synced_epic_params[:due_date] = params[:due_date]
-        end
-      end
-    end
-  end
-end

ee/app/services/work_items/callbacks/color.rb

@@ -33,7 +33,6 @@ def handle_color_change
 
         if color.valid?
           work_item.color = color
-          synced_epic_params[:color] = ::Gitlab::Color.of(params[:color])
         else
           raise_error(color.errors.full_messages.join(', '))
         end

ee/spec/services/ee/work_items/callbacks/description_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe WorkItems::Callbacks::Description, feature_category: :portfolio_management do
-  let_it_be(:random_user) { create(:user) }
-  let_it_be(:author) { create(:user) }
-  let_it_be(:project) { create(:project, :public) }
-  let_it_be(:guest) { create(:user, guest_of: project) }
-  let_it_be(:reporter) { create(:user, reporter_of: project) }
-
-  let(:params) { { description: 'updated description' } }
-  let(:current_user) { author }
-  let(:work_item) do
-    create(
-      :work_item,
-      author: author,
-      project: project,
-      description: 'old description',
-      last_edited_at: Date.yesterday,
-      last_edited_by: random_user
-    )
-  end
-
-  describe '#after_initialize' do
-    let(:service) { described_class.new(issuable: work_item, current_user: current_user, params: params) }
-
-    subject(:after_initialize_callback) { service.after_initialize }
-
-    shared_examples 'sets synced_epic_params' do
-      it 'set the synced_epic_params' do
-        subject
-
-        expect(service.synced_epic_params[:description]).to eq(params[:description])
-        expect(service.synced_epic_params[:description_html]).to eq(work_item.description_html)
-      end
-    end
-
-    shared_examples 'does not set synced_epic_params' do
-      it 'does not set the synced_epic_params' do
-        subject
-
-        expect(service.synced_epic_params[:description]).to be_nil
-        expect(service.synced_epic_params[:description_html]).to be_nil
-      end
-    end
-
-    context 'when user has permission to update description' do
-      context 'when user is work item author' do
-        let(:current_user) { author }
-
-        it_behaves_like 'sets synced_epic_params'
-      end
-
-      context 'when user is a project reporter' do
-        let(:current_user) { reporter }
-
-        it_behaves_like 'sets synced_epic_params'
-      end
-
-      context 'when description is nil' do
-        let(:current_user) { author }
-        let(:params) { { description: nil } }
-
-        it_behaves_like 'sets synced_epic_params'
-      end
-
-      context 'when description is empty' do
-        let(:current_user) { author }
-        let(:params) { { description: '' } }
-
-        it_behaves_like 'sets synced_epic_params'
-      end
-
-      context 'when description param is not present' do
-        let(:params) { {} }
-
-        it_behaves_like 'does not set synced_epic_params'
-      end
-    end
-
-    context 'when user does not have permission to update description' do
-      context 'when user is a project guest' do
-        let(:current_user) { guest }
-
-        it_behaves_like 'does not set synced_epic_params'
-      end
-
-      context 'with private project' do
-        let_it_be(:project) { create(:project) }
-
-        context 'when user is work item author' do
-          let(:current_user) { author }
-
-          it_behaves_like 'does not set synced_epic_params'
-        end
-      end
-    end
-  end
-end

ee/spec/services/ee/work_items/callbacks/start_and_due_date_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe WorkItems::Callbacks::StartAndDueDate, feature_category: :portfolio_management do
-  let_it_be(:project) { create(:project) }
-  let_it_be(:user) { create(:user, reporter_of: project) }
-  let_it_be_with_reload(:work_item) { create(:work_item, project: project) }
-
-  let(:widget) { work_item.widgets.find { |widget| widget.is_a?(WorkItems::Callbacks::StartAndDueDate) } }
-
-  describe '#before_update_callback' do
-    let(:start_date) { Date.today }
-    let(:due_date) { 1.week.from_now.to_date }
-    let(:service) { described_class.new(issuable: work_item, current_user: user, params: params) }
-
-    subject(:update_params) { service.before_update }
-
-    shared_examples 'does not set synced_epic_params' do
-      it 'does not set synced_epic_params' do
-        update_params
-
-        expect(service.synced_epic_params).to be_empty
-      end
-    end
-
-    context 'when start and due date params are present' do
-      let(:params) { { start_date: Date.today, due_date: 1.week.from_now.to_date } }
-
-      it 'correctly sets synced_epic_params' do
-        update_params
-
-        expect(service.synced_epic_params[:start_date]).to eq(start_date)
-        expect(service.synced_epic_params[:due_date]).to eq(due_date)
-      end
-
-      context "and user doesn't have permissions to update start and due date" do
-        let_it_be(:user) { create(:user) }
-
-        it_behaves_like 'does not set synced_epic_params'
-      end
-    end
-
-    context 'when date params are not present' do
-      let(:params) { {} }
-
-      it_behaves_like 'does not set synced_epic_params'
-    end
-
-    context 'when start_date and due_date are null' do
-      context 'when one of the two params is null' do
-        let(:params) { { start_date: nil, due_date: nil } }
-
-        it 'sets only one date to null' do
-          expect(service.synced_epic_params[:start_date]).to eq(nil)
-          expect(service.synced_epic_params[:due_date]).to eq(nil)
-        end
-      end
-    end
-  end
-end

ee/spec/services/work_items/callbacks/color_spec.rb

@@ -23,24 +23,12 @@ def work_item_color
         .to not_change { work_item_color }
         .and not_change { work_item.updated_at }
     end
-
-    it 'does not set synced_epic_params' do
-      subject
-
-      expect(callback.synced_epic_params).to be_empty
-    end
   end
 
   shared_examples 'color is updated' do |color|
     it 'updates work item color value' do
       expect { subject }.to change { work_item_color }.to(color)
     end
-
-    it 'sets synced_epic_params' do
-      subject
-
-      expect(callback.synced_epic_params[:color].to_s).to eq(color)
-    end
   end
 
   shared_examples 'raises a WidgetError' do

ee/spec/services/work_items/create_service_spec.rb

@@ -136,6 +136,18 @@
 
     it_behaves_like 'syncs all data from a work_item to an epic'
 
+    context 'when creating the epic with only title and description' do
+      let(:widget_params) do
+        {
+          description_widget: {
+            description: 'new description'
+          }
+        }
+      end
+
+      it_behaves_like 'syncs all data from a work_item to an epic'
+    end
+
     context 'when creating an epic work item' do
       it 'creates the epic with correct relative_position' do
         work_item = service_result.payload[:work_item]

ee/spec/services/work_items/update_service_spec.rb

@@ -326,6 +326,18 @@
 
       it_behaves_like 'syncs all data from a work_item to an epic'
 
+      context 'when only providing title and description' do
+        let(:widget_params) do
+          {
+            description_widget: {
+              description: 'new description'
+            }
+          }
+        end
+
+        it_behaves_like 'syncs all data from a work_item to an epic'
+      end
+
       it 'syncs the data to the epic', :aggregate_failures do
         update_work_item
 
@@ -371,7 +383,7 @@
       context 'when updating the epic fails' do
         before do
           allow_next_found_instance_of(Epic) do |epic|
-            allow(epic).to receive(:update!).and_raise(ActiveRecord::RecordInvalid.new)
+            allow(epic).to receive(:save!).and_raise(ActiveRecord::RecordInvalid.new)
           end
         end