fixed transaction errorccr/51520_change_milestone_email

23 files changed, 45 insertions, 51 deletions

diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 3e8b9f84042..c388913ae65 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -3,6 +3,14 @@
 class IssuableBaseService < BaseService
   private
 
+  attr_accessor :params, :skip_milestone_email
+
+  def initialize(project, user = nil, params = {})
+    super
+
+    @skip_milestone_email = @params.delete(:skip_milestone_email)
+  end
+
   def filter_params(issuable)
     ability_name = :"admin_#{issuable.to_ability_name}"
 
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index f868ec9c189..fba252b0bae 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -94,12 +94,14 @@ module Issues
     private
 
     def handle_milestone_change(issue)
+      return if skip_milestone_email
+
       return unless issue.previous_changes.include?('milestone_id')
 
-      if issue.milestone.present?
-        notification_service.async.changed_milestone_issue(issue, issue.milestone, current_user)
-      else
+      if issue.milestone.nil?
         notification_service.async.removed_milestone_issue(issue, current_user)
+      else
+        notification_service.async.changed_milestone_issue(issue, issue.milestone, current_user)
       end
     end
 
diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb
index 0f1bd67fd53..aacaf10d09c 100644
--- a/app/services/merge_requests/update_service.rb
+++ b/app/services/merge_requests/update_service.rb
@@ -108,12 +108,14 @@ module MergeRequests
     private
 
     def handle_milestone_change(merge_request)
-      if merge_request.previous_changes.include?('milestone_id')
-        if merge_request.milestone.present?
-          notification_service.async.changed_milestone_merge_request(merge_request, merge_request.milestone, current_user)
-        else
-          notification_service.async.removed_milestone_merge_request(merge_request, current_user)
-        end
+      return if skip_milestone_email
+
+      return unless merge_request.previous_changes.include?('milestone_id')
+
+      if merge_request.milestone.nil?
+        notification_service.async.removed_milestone_merge_request(merge_request, current_user)
+      else
+        notification_service.async.changed_milestone_merge_request(merge_request, merge_request.milestone, current_user)
       end
     end
 
diff --git a/app/services/milestones/destroy_service.rb b/app/services/milestones/destroy_service.rb
index 7cda802c120..87c7a282081 100644
--- a/app/services/milestones/destroy_service.rb
+++ b/app/services/milestones/destroy_service.rb
@@ -4,7 +4,7 @@ module Milestones
   class DestroyService < Milestones::BaseService
     def execute(milestone)
       Milestone.transaction do
-        update_params = { milestone: nil }
+        update_params = { milestone: nil, skip_milestone_email: true }
 
         milestone.issues.each do |issue|
           Issues::UpdateService.new(parent, current_user, update_params).execute(issue)
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index 8d1cadf31e1..fb9c18ea75d 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -146,7 +146,6 @@ class NotificationService
   #  * users with custom level checked with "new merge request"
   #
   # In EE, approvers of the merge request are also included
-
   def new_merge_request(merge_request, current_user)
     new_resource_email(merge_request, :new_merge_request_email)
   end
diff --git a/app/views/notify/_changed_milestone_issue_email.html.haml b/app/views/notify/_changed_milestone_issue_email.html.haml
deleted file mode 100644
index 4f5ba6f7e6d..00000000000
--- a/app/views/notify/_changed_milestone_issue_email.html.haml
+++ /dev/null
@@ -1,3 +0,0 @@
-%p
-  #{'Milestone'} changed to
-  %strong= link_to(@milestone.name, @milestone_url)
diff --git a/app/views/notify/_changed_milestone_issue_email.text.erb b/app/views/notify/_changed_milestone_issue_email.text.erb
deleted file mode 100644
index c5fc0b61518..00000000000
--- a/app/views/notify/_changed_milestone_issue_email.text.erb
+++ /dev/null
@@ -1 +0,0 @@
-Milestone changed to <%= @milestone.name %> ( <%= @milestone_url %> )
diff --git a/app/views/notify/_changed_milestone_merge_request_email.html.haml b/app/views/notify/_changed_milestone_merge_request_email.html.haml
deleted file mode 100644
index 4f5ba6f7e6d..00000000000
--- a/app/views/notify/_changed_milestone_merge_request_email.html.haml
+++ /dev/null
@@ -1,3 +0,0 @@
-%p
-  #{'Milestone'} changed to
-  %strong= link_to(@milestone.name, @milestone_url)
diff --git a/app/views/notify/_changed_milestone_merge_request_email.text.erb b/app/views/notify/_changed_milestone_merge_request_email.text.erb
deleted file mode 100644
index c5fc0b61518..00000000000
--- a/app/views/notify/_changed_milestone_merge_request_email.text.erb
+++ /dev/null
@@ -1 +0,0 @@
-Milestone changed to <%= @milestone.name %> ( <%= @milestone_url %> )
diff --git a/app/views/notify/_removed_milestone_issue_email.html.haml b/app/views/notify/_removed_milestone_issue_email.html.haml
deleted file mode 100644
index a7b76e57c34..00000000000
--- a/app/views/notify/_removed_milestone_issue_email.html.haml
+++ /dev/null
@@ -1,2 +0,0 @@
-%p
-  #{'Milestone'} removed
diff --git a/app/views/notify/_removed_milestone_issue_email.text.erb b/app/views/notify/_removed_milestone_issue_email.text.erb
deleted file mode 100644
index 0b83ed7a4c5..00000000000
--- a/app/views/notify/_removed_milestone_issue_email.text.erb
+++ /dev/null
@@ -1 +0,0 @@
-Milestone removed
diff --git a/app/views/notify/_removed_milestone_merge_request_email.html.haml b/app/views/notify/_removed_milestone_merge_request_email.html.haml
deleted file mode 100644
index a7b76e57c34..00000000000
--- a/app/views/notify/_removed_milestone_merge_request_email.html.haml
+++ /dev/null
@@ -1,2 +0,0 @@
-%p
-  #{'Milestone'} removed
diff --git a/app/views/notify/_removed_milestone_merge_request_email.text.erb b/app/views/notify/_removed_milestone_merge_request_email.text.erb
deleted file mode 100644
index 0b83ed7a4c5..00000000000
--- a/app/views/notify/_removed_milestone_merge_request_email.text.erb
+++ /dev/null
@@ -1 +0,0 @@
-Milestone removed
diff --git a/app/views/notify/changed_milestone_issue_email.html.haml b/app/views/notify/changed_milestone_issue_email.html.haml
index 20cf9238c58..7d5425fc72d 100644
--- a/app/views/notify/changed_milestone_issue_email.html.haml
+++ b/app/views/notify/changed_milestone_issue_email.html.haml
@@ -1 +1,3 @@
-= render 'changed_milestone_issue_email', issuable: @issue
+%p
+  Milestone changed to
+  %strong= link_to(@milestone.name, @milestone_url)
diff --git a/app/views/notify/changed_milestone_issue_email.text.erb b/app/views/notify/changed_milestone_issue_email.text.erb
index df4710635ae..c5fc0b61518 100644
--- a/app/views/notify/changed_milestone_issue_email.text.erb
+++ b/app/views/notify/changed_milestone_issue_email.text.erb
@@ -1 +1 @@
-<%= render 'changed_milestone_issue_email', issuable: @issue %>
+Milestone changed to <%= @milestone.name %> ( <%= @milestone_url %> )
diff --git a/app/views/notify/changed_milestone_merge_request_email.html.haml b/app/views/notify/changed_milestone_merge_request_email.html.haml
index 1cae95b6a8e..7d5425fc72d 100644
--- a/app/views/notify/changed_milestone_merge_request_email.html.haml
+++ b/app/views/notify/changed_milestone_merge_request_email.html.haml
@@ -1 +1,3 @@
-= render 'changed_milestone_merge_request_email', issuable: @merge_request
+%p
+  Milestone changed to
+  %strong= link_to(@milestone.name, @milestone_url)
diff --git a/app/views/notify/changed_milestone_merge_request_email.text.erb b/app/views/notify/changed_milestone_merge_request_email.text.erb
index 68b0fd7bf7c..c5fc0b61518 100644
--- a/app/views/notify/changed_milestone_merge_request_email.text.erb
+++ b/app/views/notify/changed_milestone_merge_request_email.text.erb
@@ -1 +1 @@
-<%= render 'changed_milestone_merge_request_email', issuable: @merge_request %>
+Milestone changed to <%= @milestone.name %> ( <%= @milestone_url %> )
diff --git a/app/views/notify/removed_milestone_issue_email.html.haml b/app/views/notify/removed_milestone_issue_email.html.haml
index d522b2a95fb..7e9205b6491 100644
--- a/app/views/notify/removed_milestone_issue_email.html.haml
+++ b/app/views/notify/removed_milestone_issue_email.html.haml
@@ -1 +1,2 @@
-= render 'removed_milestone_issue_email', issuable: @issue
+%p
+  Milestone removed
diff --git a/app/views/notify/removed_milestone_issue_email.text.erb b/app/views/notify/removed_milestone_issue_email.text.erb
index 43aa31c3615..0b83ed7a4c5 100644
--- a/app/views/notify/removed_milestone_issue_email.text.erb
+++ b/app/views/notify/removed_milestone_issue_email.text.erb
@@ -1 +1 @@
-<%= render 'removed_milestone_issue_email', issuable: @issue %>
+Milestone removed
diff --git a/app/views/notify/removed_milestone_merge_request_email.html.haml b/app/views/notify/removed_milestone_merge_request_email.html.haml
index e815c5c178d..7e9205b6491 100644
--- a/app/views/notify/removed_milestone_merge_request_email.html.haml
+++ b/app/views/notify/removed_milestone_merge_request_email.html.haml
@@ -1 +1,2 @@
-= render 'removed_milestone_merge_request_email', issuable: @merge_request
+%p
+  Milestone removed
diff --git a/app/views/notify/removed_milestone_merge_request_email.text.erb b/app/views/notify/removed_milestone_merge_request_email.text.erb
index b6472dbb179..0b83ed7a4c5 100644
--- a/app/views/notify/removed_milestone_merge_request_email.text.erb
+++ b/app/views/notify/removed_milestone_merge_request_email.text.erb
@@ -1 +1 @@
-<%= render 'removed_milestone_merge_request_email', issuable: @merge_request %>
+Milestone removed
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index 56ca677b6d5..bd519e7f077 100644
--- a/spec/services/issues/update_service_spec.rb
+++ b/spec/services/issues/update_service_spec.rb
@@ -347,7 +347,7 @@ describe Issues::UpdateService, :mailer do
         let!(:non_subscriber) { create(:user) }
 
         let!(:subscriber) do
-          create(:user).tap do |u|
+          create(:user) do |u|
             issue.toggle_subscription(u, project)
             project.add_developer(u)
           end
@@ -357,7 +357,9 @@ describe Issues::UpdateService, :mailer do
 
         it 'sends notifications for subscribers of changed milestone' do
           issue.milestone = create(:milestone)
+
           issue.save
+
           perform_enqueued_jobs do
             update_issue(milestone_id: "")
           end
@@ -371,7 +373,7 @@ describe Issues::UpdateService, :mailer do
         let!(:non_subscriber) { create(:user) }
 
         let!(:subscriber) do
-          create(:user).tap do |u|
+          create(:user) do |u|
             issue.toggle_subscription(u, project)
             project.add_developer(u)
           end
@@ -395,19 +397,6 @@ describe Issues::UpdateService, :mailer do
         end
       end
 
-      context 'when the milestone is deleted' do
-        it 'should not email anyone' do
-          issue.milestone = create(:milestone)
-          issue.save
-          perform_enqueued_jobs do
-            update_issue(milestone_id: '')
-          end
-
-          expect(issue.milestone.present?).to eq false
-          should_not_email_anyone
-        end
-      end
-
       context 'when the labels change' do
         before do
           Timecop.freeze(1.minute.from_now) do
@@ -429,7 +418,7 @@ describe Issues::UpdateService, :mailer do
       let!(:non_subscriber) { create(:user) }
 
       let!(:subscriber) do
-        create(:user).tap do |u|
+        create(:user) do |u|
           label.toggle_subscription(u, project)
           project.add_developer(u)
         end
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index 93ec298f654..1b599ba11b6 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -319,7 +319,7 @@ describe MergeRequests::UpdateService, :mailer do
         let!(:non_subscriber) { create(:user) }
 
         let!(:subscriber) do
-          create(:user).tap do |u|
+          create(:user) do |u|
             merge_request.toggle_subscription(u, project)
             project.add_developer(u)
           end
@@ -329,7 +329,9 @@ describe MergeRequests::UpdateService, :mailer do
 
         it 'sends notifications for subscribers of changed milestone' do
           merge_request.milestone = create(:milestone)
+
           merge_request.save
+
           perform_enqueued_jobs do
             update_merge_request(milestone_id: "")
           end
@@ -343,7 +345,7 @@ describe MergeRequests::UpdateService, :mailer do
         let!(:non_subscriber) { create(:user) }
 
         let!(:subscriber) do
-          create(:user).tap do |u|
+          create(:user) do |u|
             merge_request.toggle_subscription(u, project)
             project.add_developer(u)
           end