Merge branch 'jprovazn-fix-burndown-data' into 'master'

Remove duplicit events from burndown data

See merge request gitlab-org/gitlab-ee!14632

ee/app/models/burndown.rb

@@ -74,6 +74,7 @@ class Burndown
       Event
         .where(target: milestone_issues, action: [Event::CLOSED, Event::REOPENED])
         .where('created_at <= ?', end_date.end_of_day)
+        .order(:created_at)
         .group_by(&:target_id)
     end
   end
@@ -88,9 +89,19 @@ class Burndown
     events_for_issue = milestone_events_per_issue[issue.id]
     return [] unless events_for_issue
 
+    previous_action = nil
     events_for_issue.map do |event|
+      # It's possible that an event (we filter only closed or reopened actions)
+      # is followed by another event with the same action - typically if both
+      # commit and merge request closes an issue, then 'close' event may be
+      # created for both of them. We can ignore these "duplicit" events because
+      # if an event is already closed, another close action doesn't change its
+      # state.
+      next if event.action == previous_action
+
+      previous_action = event.action
       build_burndown_event(event.created_at, issue.weight, Event::ACTIONS.key(event.action).to_s)
-    end
+    end.compact
   end
 
   # If issue is closed but has no closed events, treat it as though closed on milestone start date

ee/changelogs/unreleased/jprovazn-fix-burndown-data.yml

+---
+title: Fix negative values in burndown charts.
+merge_request: 14632
+author:
+type: fixed

ee/spec/models/burndown_spec.rb

@@ -18,7 +18,7 @@ describe Burndown do
 
     subject { described_class.new(milestone, user).as_json }
 
-    it 'generates an array of issues with with date, issue weight and action' do
+    it 'generates an array of issues with date, issue weight and action' do
       expect(subject).to match_array([
         { created_at: Date.new(2017, 2, 28).beginning_of_day, weight: 2, action: 'created' },
         { created_at: Date.new(2017, 2, 28).beginning_of_day, weight: 2, action: 'closed' },
@@ -98,7 +98,16 @@ describe Burndown do
       end
     end
 
-    context "when all closed issues does not have closed events" do
+    it "ignores follow-up events with the same action" do
+      create(:event, target: milestone.issues.first, created_at: milestone.start_date + 1.minute, action: Event::REOPENED)
+      event1 = create(:closed_issue_event, target: milestone.issues.first, created_at: milestone.start_date + 2.minutes)
+      event2 = create(:closed_issue_event, target: milestone.issues.first, created_at: milestone.start_date + 3.minutes)
+
+      expect(closed_at_time(subject, event1.created_at).size).to eq(1)
+      expect(closed_at_time(subject, event2.created_at).size).to eq(0)
+    end
+
+    context "when all closed issues do not have closed events" do
       before do
         Event.where(target: milestone.issues, action: Event::CLOSED).destroy_all # rubocop: disable DestroyAll
       end
@@ -254,4 +263,10 @@ describe Burndown do
   def adjust_issue_event_creation_time(event)
     event.update!(created_at: event.created_at.beginning_of_day)
   end
+
+  def closed_at_time(events, time)
+    events.select do |hash|
+      hash[:created_at] == time && hash[:action] == 'closed'
+    end
+  end
 end