Commit a272c572 authored by Dan Jensen's avatar Dan Jensen

Fix missing amount in Contribution Analytics

Previously Contribution Analytics did not display closed merge
requests, so when closed merge requests existed the total would be
larger than the sum of the columns. That was interpreted as a bug
because it was unexplained.

This adds a "Closed MRs" column to the Contribution Analytics table,
so the sum of the columns always equals the total column.

Changelog: changed
EE: true
parent fed6566c
...@@ -55,6 +55,7 @@ Contributions per group member are also presented in tabular format. Click a col ...@@ -55,6 +55,7 @@ Contributions per group member are also presented in tabular format. Click a col
- Number of closed issues - Number of closed issues
- Number of opened MRs - Number of opened MRs
- Number of merged MRs - Number of merged MRs
- Number of closed MRs
- Number of total contributions - Number of total contributions
![Contribution analytics contributions table](img/group_stats_table.png) ![Contribution analytics contributions table](img/group_stats_table.png)
......
...@@ -23,6 +23,7 @@ export default { ...@@ -23,6 +23,7 @@ export default {
<td>{{ row.mergeRequestsCreated }}</td> <td>{{ row.mergeRequestsCreated }}</td>
<td>{{ row.mergeRequestsApproved }}</td> <td>{{ row.mergeRequestsApproved }}</td>
<td>{{ row.mergeRequestsMerged }}</td> <td>{{ row.mergeRequestsMerged }}</td>
<td>{{ row.mergeRequestsClosed }}</td>
<td>{{ row.totalEvents }}</td> <td>{{ row.totalEvents }}</td>
</tr> </tr>
</tbody> </tbody>
......
...@@ -8,6 +8,7 @@ const COLUMNS = [ ...@@ -8,6 +8,7 @@ const COLUMNS = [
{ name: 'mergeRequestsCreated', text: __('Opened MRs') }, { name: 'mergeRequestsCreated', text: __('Opened MRs') },
{ name: 'mergeRequestsApproved', text: __('Approved MRs') }, { name: 'mergeRequestsApproved', text: __('Approved MRs') },
{ name: 'mergeRequestsMerged', text: __('Merged MRs') }, { name: 'mergeRequestsMerged', text: __('Merged MRs') },
{ name: 'mergeRequestsClosed', text: __('Closed MRs') },
{ name: 'totalEvents', text: __('Total Contributions') }, { name: 'totalEvents', text: __('Total Contributions') },
]; ];
......
...@@ -42,10 +42,11 @@ ...@@ -42,10 +42,11 @@
%div{ data: { qa_selector: 'merge_request_content' } } %div{ data: { qa_selector: 'merge_request_content' } }
%h3= s_('ContributionAnalytics|Merge requests') %h3= s_('ContributionAnalytics|Merge requests')
- mr_closed_count = @data_collector.total_merge_requests_closed_count
- mr_created_count = @data_collector.total_merge_requests_created_count - mr_created_count = @data_collector.total_merge_requests_created_count
- mr_merged_count = @data_collector.total_merge_requests_merged_count - mr_merged_count = @data_collector.total_merge_requests_merged_count
- if mr_created_count > 0 || mr_merged_count > 0 - if mr_closed_count > 0 || mr_created_count > 0 || mr_merged_count > 0
= html_escape(s_('ContributionAnalytics|%{created_count} created, %{merged_count} merged.')) % { created_count: tag.strong(mr_created_count), merged_count: tag.strong(mr_merged_count) } = html_escape(s_('ContributionAnalytics|%{created_count} created, %{merged_count} merged, %{closed_count} closed.')) % { mr_closed_count: tag.strong(mr_closed_count), created_count: tag.strong(mr_created_count), merged_count: tag.strong(mr_merged_count) }
- else - else
= s_('ContributionAnalytics|No merge requests for the selected time period.') = s_('ContributionAnalytics|No merge requests for the selected time period.')
......
...@@ -5,7 +5,7 @@ ...@@ -5,7 +5,7 @@
module Gitlab module Gitlab
module ContributionAnalytics module ContributionAnalytics
class DataCollector class DataCollector
EVENT_TYPES = %i[push issues_created issues_closed merge_requests_created merge_requests_merged merge_requests_approved total_events].freeze EVENT_TYPES = %i[push issues_created issues_closed merge_requests_closed merge_requests_created merge_requests_merged merge_requests_approved total_events].freeze
attr_reader :group, :from attr_reader :group, :from
...@@ -32,6 +32,12 @@ module Gitlab ...@@ -32,6 +32,12 @@ module Gitlab
end end
end end
def merge_requests_closed_by_author_count
all_counts.each_with_object({}) do |(event, count), hash|
hash[event.author_id] = count if event.merge_request? && event.closed_action?
end
end
def merge_requests_created_by_author_count def merge_requests_created_by_author_count
all_counts.each_with_object({}) do |(event, count), hash| all_counts.each_with_object({}) do |(event, count), hash|
hash[event.author_id] = count if event.merge_request? && event.created_action? hash[event.author_id] = count if event.merge_request? && event.created_action?
...@@ -69,6 +75,10 @@ module Gitlab ...@@ -69,6 +75,10 @@ module Gitlab
PushEventPayload.commit_count_for(base_query.pushed_action) PushEventPayload.commit_count_for(base_query.pushed_action)
end end
def total_merge_requests_closed_count
all_counts.sum { |event, count| event.merge_request? && event.closed_action? ? count : 0 }
end
def total_merge_requests_created_count def total_merge_requests_created_count
all_counts.sum { |event, count| event.merge_request? && event.created_action? ? count : 0 } all_counts.sum { |event, count| event.merge_request? && event.created_action? ? count : 0 }
end end
...@@ -111,6 +121,7 @@ module Gitlab ...@@ -111,6 +121,7 @@ module Gitlab
push: push_by_author_count, push: push_by_author_count,
issues_created: issues_created_by_author_count, issues_created: issues_created_by_author_count,
issues_closed: issues_closed_by_author_count, issues_closed: issues_closed_by_author_count,
merge_requests_closed: merge_requests_closed_by_author_count,
merge_requests_created: merge_requests_created_by_author_count, merge_requests_created: merge_requests_created_by_author_count,
merge_requests_merged: merge_requests_merged_by_author_count, merge_requests_merged: merge_requests_merged_by_author_count,
merge_requests_approved: merge_requests_approved_by_author_count, merge_requests_approved: merge_requests_approved_by_author_count,
......
...@@ -32,7 +32,7 @@ describe('TableBodyComponent', () => { ...@@ -32,7 +32,7 @@ describe('TableBodyComponent', () => {
const rowEl = vm.$el.querySelector('tr'); const rowEl = vm.$el.querySelector('tr');
expect(rowEl).not.toBeNull(); expect(rowEl).not.toBeNull();
expect(rowEl.querySelectorAll('td')).toHaveLength(8); expect(rowEl.querySelectorAll('td')).toHaveLength(9);
}); });
it('renders username row cell element', () => { it('renders username row cell element', () => {
......
...@@ -10,6 +10,7 @@ export const rawMembers = [ ...@@ -10,6 +10,7 @@ export const rawMembers = [
issues_closed: 4, issues_closed: 4,
merge_requests_created: 2, merge_requests_created: 2,
merge_requests_merged: 0, merge_requests_merged: 0,
merge_requests_closed: 0,
total_events: 51, total_events: 51,
}, },
{ {
...@@ -21,6 +22,7 @@ export const rawMembers = [ ...@@ -21,6 +22,7 @@ export const rawMembers = [
issues_closed: 1, issues_closed: 1,
merge_requests_created: 5, merge_requests_created: 5,
merge_requests_merged: 0, merge_requests_merged: 0,
merge_requests_closed: 0,
total_events: 49, total_events: 49,
}, },
{ {
...@@ -32,7 +34,8 @@ export const rawMembers = [ ...@@ -32,7 +34,8 @@ export const rawMembers = [
issues_closed: 1, issues_closed: 1,
merge_requests_created: 1, merge_requests_created: 1,
merge_requests_merged: 0, merge_requests_merged: 0,
total_events: 44, merge_requests_closed: 1,
total_events: 45,
}, },
]; ];
...@@ -40,6 +43,7 @@ export const mockSortOrders = { ...@@ -40,6 +43,7 @@ export const mockSortOrders = {
fullname: 1, fullname: 1,
issuesClosed: 1, issuesClosed: 1,
issuesCreated: 1, issuesCreated: 1,
mergeRequestsClosed: 1,
mergeRequestsCreated: 1, mergeRequestsCreated: 1,
mergeRequestsApproved: 1, mergeRequestsApproved: 1,
mergeRequestsMerged: 1, mergeRequestsMerged: 1,
......
...@@ -16,6 +16,7 @@ RSpec.describe Gitlab::ContributionAnalytics::DataCollector do ...@@ -16,6 +16,7 @@ RSpec.describe Gitlab::ContributionAnalytics::DataCollector do
create(:event, :closed, project: project1, target: issue, author: user) create(:event, :closed, project: project1, target: issue, author: user)
create(:event, :created, project: project2, target: mr, author: user) create(:event, :created, project: project2, target: mr, author: user)
create(:event, :approved, project: project2, target: mr, author: user) create(:event, :approved, project: project2, target: mr, author: user)
create(:event, :closed, project: project2, target: mr, author: user)
data_collector = described_class.new(group: group) data_collector = described_class.new(group: group)
expect(data_collector.totals).to eq({ expect(data_collector.totals).to eq({
...@@ -24,8 +25,9 @@ RSpec.describe Gitlab::ContributionAnalytics::DataCollector do ...@@ -24,8 +25,9 @@ RSpec.describe Gitlab::ContributionAnalytics::DataCollector do
merge_requests_created: { user.id => 1 }, merge_requests_created: { user.id => 1 },
merge_requests_merged: {}, merge_requests_merged: {},
merge_requests_approved: { user.id => 1 }, merge_requests_approved: { user.id => 1 },
merge_requests_closed: { user.id => 1 },
push: {}, push: {},
total_events: { user.id => 3 } total_events: { user.id => 4 }
}) })
end end
end end
...@@ -40,6 +42,7 @@ RSpec.describe Gitlab::ContributionAnalytics::DataCollector do ...@@ -40,6 +42,7 @@ RSpec.describe Gitlab::ContributionAnalytics::DataCollector do
[5, MergeRequest.name, Event.actions[:created]] => 0, [5, MergeRequest.name, Event.actions[:created]] => 0,
[6, MergeRequest.name, Event.actions[:created]] => 1, [6, MergeRequest.name, Event.actions[:created]] => 1,
[6, MergeRequest.name, Event.actions[:approved]] => 1, [6, MergeRequest.name, Event.actions[:approved]] => 1,
[6, MergeRequest.name, Event.actions[:closed]] => 1,
[10, Issue.name, Event.actions[:closed]] => 10, [10, Issue.name, Event.actions[:closed]] => 10,
[11, Issue.name, Event.actions[:closed]] => 11 [11, Issue.name, Event.actions[:closed]] => 11
} }
...@@ -64,6 +67,10 @@ RSpec.describe Gitlab::ContributionAnalytics::DataCollector do ...@@ -64,6 +67,10 @@ RSpec.describe Gitlab::ContributionAnalytics::DataCollector do
expect(data_collector.total_push_count).to eq(4) expect(data_collector.total_push_count).to eq(4)
end end
it 'for #total_merge_requests_closed_count' do
expect(data_collector.total_merge_requests_closed_count).to eq(1)
end
it 'for #total_merge_requests_created_count' do it 'for #total_merge_requests_created_count' do
expect(data_collector.total_merge_requests_created_count).to eq(1) expect(data_collector.total_merge_requests_created_count).to eq(1)
end end
......
...@@ -9,6 +9,7 @@ RSpec.describe UserAnalyticsEntity do ...@@ -9,6 +9,7 @@ RSpec.describe UserAnalyticsEntity do
push: {}, push: {},
issues_created: {}, issues_created: {},
issues_closed: {}, issues_closed: {},
merge_requests_closed: {},
merge_requests_created: {}, merge_requests_created: {},
merge_requests_merged: {}, merge_requests_merged: {},
merge_requests_approved: {}, merge_requests_approved: {},
......
...@@ -6925,6 +6925,9 @@ msgstr "" ...@@ -6925,6 +6925,9 @@ msgstr ""
msgid "Closed %{epicTimeagoDate}" msgid "Closed %{epicTimeagoDate}"
msgstr "" msgstr ""
msgid "Closed MRs"
msgstr ""
msgid "Closed epics" msgid "Closed epics"
msgstr "" msgstr ""
...@@ -8844,7 +8847,7 @@ msgstr "" ...@@ -8844,7 +8847,7 @@ msgstr ""
msgid "ContributionAnalytics|%{created_count} created, %{closed_count} closed." msgid "ContributionAnalytics|%{created_count} created, %{closed_count} closed."
msgstr "" msgstr ""
msgid "ContributionAnalytics|%{created_count} created, %{merged_count} merged." msgid "ContributionAnalytics|%{created_count} created, %{merged_count} merged, %{closed_count} closed."
msgstr "" msgstr ""
msgid "ContributionAnalytics|%{pushes} pushes, more than %{commits} commits by %{people} contributors." msgid "ContributionAnalytics|%{pushes} pushes, more than %{commits} commits by %{people} contributors."
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment