Merge branch 'fix-issue-2382' into 'master'

Create a "destroyed Milestone" event and keep Milestone events around in the DB for posterity

Also fix issue where destroying a Milestone would cause odd, transient messages like "created milestone" or "imported milestone".

Now if a milestone is destroyed, at least it will indicate in the activity feed even if it's not clear which milestone was destroyed:

![image](https://gitlab.com/gitlab-org/gitlab-ce/uploads/c89cc8a0a9fa549deac433f17b890913/image.png)

Closes #2382

See merge request !1227

app/controllers/projects/milestones_controller.rb

@@ -66,12 +66,7 @@ class Projects::MilestonesController < Projects::ApplicationController
   def destroy
     return access_denied! unless can?(current_user, :admin_milestone, @project)
 
-    update_params = { milestone: nil }
-    @milestone.issues.each do |issue|
-      Issues::UpdateService.new(@project, current_user, update_params).execute(issue)
-    end
-
-    @milestone.destroy
+    Milestones::DestroyService.new(project, current_user).execute(milestone)
 
     respond_to do |format|
       format.html { redirect_to namespace_project_milestones_path }

app/helpers/events_helper.rb

@@ -46,6 +46,14 @@ module EventsHelper
     }
   end
 
+  def event_preposition(event)
+    if event.push? || event.commented? || event.target
+      "at"
+    elsif event.milestone?
+      "in"
+    end
+  end
+
   def event_feed_title(event)
     words = []
     words << event.author_name
@@ -62,6 +70,9 @@ module EventsHelper
         words << "##{truncate event.note_target_iid}"
       end
       words << "at"
+    elsif event.milestone?
+      words << "##{event.target_iid}" if event.target_iid
+      words << "in"
     elsif event.target
       words << "##{event.target_iid}:"
       words << event.target.title if event.target.respond_to?(:title)

app/models/event.rb

@@ -27,6 +27,7 @@ class Event < ActiveRecord::Base
   MERGED    = 7
   JOINED    = 8 # User joined project
   LEFT      = 9 # User left project
+  DESTROYED = 10
 
   delegate :name, :email, to: :author, prefix: true, allow_nil: true
   delegate :title, to: :issue, prefix: true, allow_nil: true
@@ -48,6 +49,7 @@ class Event < ActiveRecord::Base
   scope :code_push, -> { where(action: PUSHED) }
   scope :in_projects, ->(project_ids) { where(project_id: project_ids).recent }
   scope :with_associations, -> { includes(project: :namespace) }
+  scope :for_milestone_id, ->(milestone_id) { where(target_type: "Milestone", target_id: milestone_id) }
 
   class << self
     def reset_event_cache_for(target)
@@ -71,7 +73,7 @@ class Event < ActiveRecord::Base
     elsif created_project?
       true
     else
-      (issue? || merge_request? || note? || milestone?) && target
+      ((issue? || merge_request? || note?) && target) || milestone?
     end
   end
 
@@ -115,6 +117,10 @@ class Event < ActiveRecord::Base
     action == LEFT
   end
 
+  def destroyed?
+    action == DESTROYED
+  end
+
   def commented?
     action == COMMENTED
   end
@@ -124,7 +130,7 @@ class Event < ActiveRecord::Base
   end
 
   def created_project?
-    created? && !target
+    created? && !target && target_type.nil?
   end
 
   def created_target?
@@ -180,6 +186,8 @@ class Event < ActiveRecord::Base
       'joined'
     elsif left?
       'left'
+    elsif destroyed?
+      'destroyed'
     elsif commented?
       "commented on"
     elsif created_project?

app/models/milestone.rb

@@ -61,7 +61,7 @@ class Milestone < ActiveRecord::Base
       false
     end
   end
-  
+
   def open_items_count
     self.issues.opened.count + self.merge_requests.opened.count
   end

app/services/event_create_service.rb

@@ -46,6 +46,10 @@ class EventCreateService
     create_record_event(milestone, current_user, Event::REOPENED)
   end
 
+  def destroy_milestone(milestone, current_user)
+    create_record_event(milestone, current_user, Event::DESTROYED)
+  end
+
   def leave_note(note, current_user)
     create_record_event(note, current_user, Event::COMMENTED)
   end

app/services/milestones/destroy_service.rb

@@ -0,0 +1,22 @@
+module Milestones
+  class DestroyService < Milestones::BaseService
+    def execute(milestone)
+
+      Milestone.transaction do
+        update_params = { milestone: nil }
+        milestone.issues.each do |issue|
+          Issues::UpdateService.new(project, current_user, update_params).execute(issue)
+        end
+
+        event_service.destroy_milestone(milestone, current_user)
+
+        Event.for_milestone_id(milestone.id).each do |event|
+          event.target_id = nil
+          event.save
+        end
+
+        milestone.destroy
+      end
+    end
+  end
+end

app/views/events/event/_common.html.haml

@@ -5,13 +5,14 @@
 
   - if event.target
     %strong= link_to "##{event.target_iid}", [event.project.namespace.becomes(Namespace), event.project, event.target]
-    at
+
+  = event_preposition(event)
 
   - if event.project
     = link_to_project event.project
   - else
     = event.project_name
-    
+
 - if event.target.respond_to?(:title)
   .event-body
     .event-note

features/project/issues/milestones.feature

@@ -12,13 +12,17 @@ Feature: Project Issues Milestones
     Given I click link "v2.2"
     Then I should see milestone "v2.2"
 
-  Scenario: I create new milestone
+  @javascript
+  Scenario: I create and delete new milestone
     Given I click link "New Milestone"
     And I submit new milestone "v2.3"
     Then I should see milestone "v2.3"
+    Given I click link to remove milestone
+    When I visit project "Shop" activity page
+    Then I should see deleted milestone activity
 
   Scenario: I delete new milestone
-    Given I click link to remove milestone "v2.2"
+    Given I click link to remove milestone
     And I should see no milestones
 
   @javascript

features/steps/project/issues/milestones.rb

@@ -49,6 +49,11 @@ class Spinach::Features::ProjectIssuesMilestones < Spinach::FeatureSteps
     create(:closed_issue, project: project, milestone: milestone)
   end
 
+  step 'I should see deleted milestone activity' do
+    expect(page).to have_content('opened milestone in')
+    expect(page).to have_content('destroyed milestone in')
+  end
+
   When 'I click link "All Issues"' do
     click_link 'All Issues'
   end
@@ -57,7 +62,7 @@ class Spinach::Features::ProjectIssuesMilestones < Spinach::FeatureSteps
     expect(page).to have_selector('#tab-issues li.issue-row', count: 4)
   end
 
-  step 'I click link to remove milestone "v2.2"' do
+  step 'I click link to remove milestone' do
     click_link 'Remove'
   end
 

features/steps/shared/project.rb

@@ -51,6 +51,11 @@ module SharedProject
     visit namespace_project_path(project.namespace, project)
   end
 
+  step 'I visit project "Shop" activity page' do
+    project = Project.find_by(name: 'Shop')
+    visit namespace_project_path(project.namespace, project)
+  end
+
   step 'project "Shop" has push event' do
     @project = Project.find_by(name: "Shop")
 

spec/controllers/projects/milestones_controller_spec.rb

@@ -15,8 +15,12 @@ describe Projects::MilestonesController do
   describe "#destroy" do
     it "should remove milestone" do
       expect(issue.milestone_id).to eq(milestone.id)
+
       delete :destroy, namespace_id: project.namespace.id, project_id: project.id, id: milestone.id, format: :js
       expect(response).to be_success
+
+      expect(Event.first.action).to eq(Event::DESTROYED)
+
       expect { Milestone.find(milestone.id) }.to raise_exception(ActiveRecord::RecordNotFound)
       issue.reload
       expect(issue.milestone_id).to eq(nil)

spec/services/event_create_service_spec.rb

@@ -99,5 +99,15 @@ describe EventCreateService do
         expect { service.close_milestone(milestone, user) }.to change { Event.count }
       end
     end
+
+    describe :destroy_mr do
+      let(:milestone) { create(:milestone) }
+
+      it { expect(service.destroy_milestone(milestone, user)).to be_truthy }
+
+      it "should create new event" do
+        expect { service.destroy_milestone(milestone, user) }.to change { Event.count }
+      end
+    end
   end
 end