Merge branch '38096-refactor-change-milestone-service-pd' into 'master'

Refactor constructor and specs of ChangeMilestoneService

See merge request gitlab-org/gitlab!25560

app/services/issuable/common_system_notes_service.rb

@@ -101,7 +101,7 @@ module Issuable
     def create_milestone_note
       if milestone_changes_tracking_enabled?
         # Creates a synthetic note
-        ResourceEvents::ChangeMilestoneService.new(resource: issuable, user: current_user).execute
+        ResourceEvents::ChangeMilestoneService.new(issuable, current_user).execute
       else
         SystemNoteService.change_milestone(issuable, issuable.project, current_user, issuable.milestone)
       end

app/services/resource_events/change_milestone_service.rb

@@ -4,7 +4,7 @@ module ResourceEvents
   class ChangeMilestoneService
     attr_reader :resource, :user, :event_created_at, :milestone
 
-    def initialize(resource:, user:, created_at: Time.now)
+    def initialize(resource, user, created_at: Time.now)
       @resource = resource
       @user = user
       @event_created_at = created_at

spec/services/resource_events/change_milestone_service_spec.rb

@@ -3,65 +3,11 @@
 require 'spec_helper'
 
 describe ResourceEvents::ChangeMilestoneService do
-  shared_examples 'milestone events creator' do
-    let_it_be(:user) { create(:user) }
-
-    let_it_be(:milestone) { create(:milestone) }
-
-    context 'when milestone is present' do
-      before do
-        resource.milestone = milestone
-      end
-
-      let(:service) { described_class.new(resource: resource, user: user, created_at: created_at_time) }
-
-      it 'creates the expected event record' do
-        expect { service.execute }.to change { ResourceMilestoneEvent.count }.from(0).to(1)
-
-        events = ResourceMilestoneEvent.all
-
-        expect(events.size).to eq(1)
-        expect_event_record(events.first, action: 'add', milestone: milestone, state: 'opened')
-      end
-    end
-
-    context 'when milestones is not present' do
-      before do
-        resource.milestone = nil
-      end
-
-      let(:service) { described_class.new(resource: resource, user: user, created_at: created_at_time) }
-
-      it 'creates the expected event records' do
-        expect { service.execute }.to change { ResourceMilestoneEvent.count }.from(0).to(1)
-
-        expect_event_record(ResourceMilestoneEvent.first, action: 'remove', milestone: nil, state: 'opened')
-      end
-    end
-
-    def expect_event_record(event, expected_attrs)
-      expect(event.action).to eq(expected_attrs[:action])
-      expect(event.state).to eq(expected_attrs[:state])
-      expect(event.user).to eq(user)
-      expect(event.issue).to eq(resource) if resource.is_a?(Issue)
-      expect(event.issue).to be_nil unless resource.is_a?(Issue)
-      expect(event.merge_request).to eq(resource) if resource.is_a?(MergeRequest)
-      expect(event.merge_request).to be_nil unless resource.is_a?(MergeRequest)
-      expect(event.milestone).to eq(expected_attrs[:milestone])
-      expect(event.created_at).to eq(created_at_time)
-    end
-  end
-
-  let_it_be(:merge_request) { create(:merge_request) }
-  let_it_be(:issue) { create(:issue) }
-
-  let!(:created_at_time) { Time.utc(2019, 12, 30) }
-
-  it_behaves_like 'milestone events creator' do
-    let(:resource) { issue }
+  it_behaves_like 'a milestone events creator' do
+    let(:resource) { create(:issue) }
   end
 
-  it_behaves_like 'milestone events creator' do
-    let(:resource) { merge_request }
+  it_behaves_like 'a milestone events creator' do
+    let(:resource) { create(:merge_request) }
   end
 end

spec/support/shared_examples/services/resource_events/change_milestone_service_shared_examples.rb

+# frozen_string_literal: true
+
+shared_examples 'a milestone events creator' do
+  let_it_be(:user) { create(:user) }
+
+  let(:created_at_time) { Time.utc(2019, 12, 30) }
+  let(:service) { described_class.new(resource, user, created_at: created_at_time) }
+
+  context 'when milestone is present' do
+    let_it_be(:milestone) { create(:milestone) }
+
+    before do
+      resource.milestone = milestone
+    end
+
+    it 'creates the expected event record' do
+      expect { service.execute }.to change { ResourceMilestoneEvent.count }.by(1)
+
+      expect_event_record(ResourceMilestoneEvent.last, action: 'add', milestone: milestone, state: 'opened')
+    end
+  end
+
+  context 'when milestones is not present' do
+    before do
+      resource.milestone = nil
+    end
+
+    it 'creates the expected event records' do
+      expect { service.execute }.to change { ResourceMilestoneEvent.count }.by(1)
+
+      expect_event_record(ResourceMilestoneEvent.last, action: 'remove', milestone: nil, state: 'opened')
+    end
+  end
+
+  def expect_event_record(event, expected_attrs)
+    expect(event.action).to eq(expected_attrs[:action])
+    expect(event.state).to eq(expected_attrs[:state])
+    expect(event.user).to eq(user)
+    expect(event.issue).to eq(resource) if resource.is_a?(Issue)
+    expect(event.issue).to be_nil unless resource.is_a?(Issue)
+    expect(event.merge_request).to eq(resource) if resource.is_a?(MergeRequest)
+    expect(event.merge_request).to be_nil unless resource.is_a?(MergeRequest)
+    expect(event.milestone).to eq(expected_attrs[:milestone])
+    expect(event.created_at).to eq(created_at_time)
+  end
+end