Commit 1b0c13f7 authored by Rémy Coutable's avatar Rémy Coutable

Allow to specify `period_field` in Insights config

Supported values are:

- `created_at` (default): Group data using the `created_at` field.
- `closed_at`: Group data using the `closed_at` field (for issues only).
- `merged_at`: Group data using the `merged_at` field (for merge
  requests only).

The `period_field` is automatically set to:

- `closed_at` if `query.issuable_state` is `closed`
- `merged_at` if `query.issuable_state` is `merged`
- `created_at` otherwise
Signed-off-by: default avatarRémy Coutable <remy@rymai.me>
parent 17140387
......@@ -271,6 +271,22 @@ you defined.
| `week` | 4 |
| `month` | 12 |
#### `query.period_field`
Define the timestamp field used to group "issuables".
Supported values are:
- `created_at` (default): Group data using the `created_at` field.
- `closed_at`: Group data using the `closed_at` field (for issues only).
- `merged_at`: Group data using the `merged_at` field (for merge requests only).
The `period_field` is automatically set to:
- `closed_at` if `query.issuable_state` is `closed`
- `merged_at` if `query.issuable_state` is `merged`
- `created_at` otherwise
### `projects`
> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/10904) in [GitLab Ultimate](https://about.gitlab.com/pricing/) 12.4.
......
......@@ -47,8 +47,12 @@ module InsightsActions
@query_param ||= params[:query] || {}
end
def period_param
@period_param ||= query_param[:group_by]
def period_field_param
@period_field_param ||= query_param[:period_field]
end
def group_by_param
@group_by_param ||= query_param[:group_by]
end
def projects_param
......@@ -61,17 +65,17 @@ module InsightsActions
def insights_json
issuables = finder.find
insights = reduce(issuables: issuables, period_limit: finder.period_limit)
insights = reduce(issuables: issuables, period_limit: finder.period_limit, period_field: period_field_param || finder.period_field)
serializer.present(insights)
end
def reduce(issuables:, period_limit: nil)
def reduce(issuables:, period_limit:, period_field:)
case type_param
when 'stacked-bar', 'line'
Gitlab::Insights::Reducers::LabelCountPerPeriodReducer.reduce(issuables, period: period_param, period_limit: period_limit, labels: collection_labels_param)
Gitlab::Insights::Reducers::LabelCountPerPeriodReducer.reduce(issuables, period: group_by_param, period_limit: period_limit, period_field: period_field, labels: collection_labels_param)
when 'bar', 'pie'
if period_param
Gitlab::Insights::Reducers::CountPerPeriodReducer.reduce(issuables, period: period_param, period_limit: period_limit)
if group_by_param
Gitlab::Insights::Reducers::CountPerPeriodReducer.reduce(issuables, period: group_by_param, period_limit: period_limit, period_field: period_field)
else
Gitlab::Insights::Reducers::CountPerLabelReducer.reduce(issuables, labels: collection_labels_param)
end
......@@ -90,7 +94,7 @@ module InsightsActions
when 'stacked-bar'
Gitlab::Insights::Serializers::Chartjs::MultiSeriesSerializer
when 'bar', 'pie'
if period_param
if group_by_param
Gitlab::Insights::Serializers::Chartjs::BarTimeSeriesSerializer
else
Gitlab::Insights::Serializers::Chartjs::BarSerializer
......
---
title: Allow to specify `period_field` in Insights config
merge_request: 33753
author:
type: added
......@@ -16,6 +16,10 @@ module Gitlab
issue: ::IssuesFinder,
merge_request: ::MergeRequestsFinder
}.with_indifferent_access.freeze
PERIOD_FIELDS = {
issue: { opened: :created_at, closed: :closed_at },
merge_request: { opened: :created_at, merged: :merged_at }
}.with_indifferent_access.freeze
PERIODS = {
days: { default: 30 },
weeks: { default: 12 },
......@@ -33,6 +37,14 @@ module Gitlab
@issuable_type ||= query[:issuable_type]&.to_s&.singularize&.to_sym
end
def issuable_state
@issuable_state ||= query[:issuable_state]&.to_s&.to_sym || :opened
end
def period_field
@period_field ||= PERIOD_FIELDS.dig(issuable_type, issuable_state) || :created_at
end
# Returns an Active Record relation of issuables.
def find
return unless entity_args
......@@ -40,6 +52,7 @@ module Gitlab
relation = finder
.new(current_user, finder_args)
.execute
relation = relation.including_metrics if period_field == :merged_at
relation = relation.preload(:labels) if query.key?(:collection_labels) # rubocop:disable CodeReuse/ActiveRecord
relation
......@@ -70,7 +83,7 @@ module Gitlab
def finder_args
{
include_subgroups: true,
state: query[:issuable_state] || 'opened',
state: issuable_state,
label_name: query[:filter_labels],
sort: 'created_asc',
created_after: created_after_argument
......
......@@ -10,7 +10,7 @@ module Gitlab
new(issuables, **args).reduce
end
def initialize(issuables)
def initialize(issuables, **_args)
@issuables = issuables
end
private_class_method :new
......@@ -20,6 +20,11 @@ module Gitlab
raise NotImplementedError
end
def issuable_type
# `issuables.class` would be `Issue::ActiveRecord_Relation` / `MergeRequest::ActiveRecord_Relation` here
@issuable_type ||= issuables.class.to_s.underscore.split('/').first.to_sym
end
private
attr_reader :issuables
......
......@@ -9,7 +9,10 @@ module Gitlab
InvalidPeriodLimitError = Class.new(BaseReducerError)
VALID_PERIOD = %w[day week month].freeze
VALID_PERIOD_FIELD = %i[created_at].freeze
VALID_PERIOD_FIELDS = {
issue: %i[created_at closed_at],
merge_request: %i[created_at merged_at]
}.with_indifferent_access.freeze
def initialize(issuables, period:, period_limit:, period_field: :created_at)
super(issuables)
......@@ -42,8 +45,8 @@ module Gitlab
raise InvalidPeriodError, "Invalid value for `period`: `#{period}`. Allowed values are #{VALID_PERIOD}!"
end
unless VALID_PERIOD_FIELD.include?(period_field)
raise InvalidPeriodFieldError, "Invalid value for `period_field`: `#{period_field}`. Allowed values are #{VALID_PERIOD_FIELD}!"
unless VALID_PERIOD_FIELDS[issuable_type].include?(period_field)
raise InvalidPeriodFieldError, "Invalid value for `period_field`: `#{period_field}`. Allowed values are #{VALID_PERIOD_FIELDS[issuable_type]}!"
end
unless period_limit > 0
......
......@@ -16,6 +16,17 @@ RSpec.describe Gitlab::Insights::Finders::IssuableFinder do
}
end
let(:merge_request_extra_attrs) do
[
{ source_branch: "add_images_and_changes" },
{ source_branch: "improve/awesome" },
{ source_branch: "feature_conflict" },
{ source_branch: "markdown" },
{ source_branch: "feature_one" },
{ source_branch: "feature_two" }
]
end
describe '#issuable_type' do
subject { described_class.new(build(:project), nil, query: { issuable_type: issuable_type_in_query }).issuable_type }
......@@ -31,17 +42,46 @@ RSpec.describe Gitlab::Insights::Finders::IssuableFinder do
end
end
describe '#find' do
def find(entity, query:, projects: {})
described_class.new(entity, nil, query: query, projects: projects).find
describe '#issuable_state' do
subject { described_class.new(build(:project), nil, query: { issuable_state: issuable_state_in_query }).issuable_state }
where(:issuable_state_in_query, :expected_issuable_state) do
nil | :opened
'opened' | :opened
'closed' | :closed
'merged' | :merged
'locked' | :locked
end
it 'calls issuable_type' do
finder = described_class.new(build(:project), nil, query: { issuable_type: 'issue' })
with_them do
it { is_expected.to eq(expected_issuable_state) }
end
end
expect(finder).to receive(:issuable_type).and_call_original
describe '#period_field' do
subject { described_class.new(build(:project), nil, query: { issuable_type: issuable_type_in_query, issuable_state: issuable_state_in_query }).period_field }
finder.find
where(:issuable_type_in_query, :issuable_state_in_query, :expected_period_field) do
'issue' | nil | :created_at
'merge_request' | nil | :created_at
'issue' | 'opened' | :created_at
'merge_request' | 'opened' | :created_at
'issue' | 'closed' | :closed_at
'merge_request' | 'closed' | :created_at
'issue' | 'merged' | :created_at
'merge_request' | 'merged' | :merged_at
'issue' | 'locked' | :created_at
'merge_request' | 'locked' | :created_at
end
with_them do
it { is_expected.to eq(expected_period_field) }
end
end
describe '#find' do
def find(entity, query:, projects: {})
described_class.new(entity, nil, query: query, projects: projects).find
end
it 'raises an error for an invalid :issuable_type option' do
......@@ -79,14 +119,16 @@ RSpec.describe Gitlab::Insights::Finders::IssuableFinder do
let(:label_create) { create(label_type, label_entity_association_key => entity, name: 'Create') }
let(:label_quality) { create(label_type, label_entity_association_key => entity, name: 'Quality') }
let(:extra_issuable_attrs) { [{}, {}, {}, {}, {}, {}] }
let!(:issuable0) { create(:"labeled_#{issuable_type.singularize}", :opened, created_at: Time.utc(2018, 1, 1), project_association_key => project, **extra_issuable_attrs[0]) }
let!(:issuable1) { create(:"labeled_#{issuable_type.singularize}", :opened, created_at: Time.utc(2018, 2, 1), labels: [label_bug, label_manage], project_association_key => project, **extra_issuable_attrs[1]) }
let!(:issuable2) { create(:"labeled_#{issuable_type.singularize}", :opened, created_at: Time.utc(2019, 2, 6), labels: [label_bug, label_plan], project_association_key => project, **extra_issuable_attrs[2]) }
let!(:issuable3) { create(:"labeled_#{issuable_type.singularize}", :opened, created_at: Time.utc(2019, 2, 20), labels: [label_bug, label_create], project_association_key => project, **extra_issuable_attrs[3]) }
let!(:issuable4) { create(:"labeled_#{issuable_type.singularize}", :opened, created_at: Time.utc(2019, 3, 5), labels: [label_bug, label_quality], project_association_key => project, **extra_issuable_attrs[4]) }
let(:issuable_state) { :opened }
let!(:issuable0) { create(:"labeled_#{issuable_type.singularize}", issuable_state, created_at: Time.utc(2018, 1, 1), project_association_key => project, **extra_issuable_attrs[0]) }
let!(:issuable1) { create(:"labeled_#{issuable_type.singularize}", issuable_state, created_at: Time.utc(2018, 2, 1), labels: [label_bug, label_manage], project_association_key => project, **extra_issuable_attrs[1]) }
let!(:issuable2) { create(:"labeled_#{issuable_type.singularize}", issuable_state, created_at: Time.utc(2019, 2, 6), labels: [label_bug, label_plan], project_association_key => project, **extra_issuable_attrs[2]) }
let!(:issuable3) { create(:"labeled_#{issuable_type.singularize}", issuable_state, created_at: Time.utc(2019, 2, 20), labels: [label_bug, label_create], project_association_key => project, **extra_issuable_attrs[3]) }
let!(:issuable4) { create(:"labeled_#{issuable_type.singularize}", issuable_state, created_at: Time.utc(2019, 3, 5), labels: [label_bug, label_quality], project_association_key => project, **extra_issuable_attrs[4]) }
let(:query) do
base_query.merge(
issuable_type: issuable_type,
issuable_state: issuable_state,
filter_labels: [label_bug.title],
collection_labels: [label_manage.title, label_plan.title, label_create.title])
end
......@@ -167,7 +209,7 @@ RSpec.describe Gitlab::Insights::Finders::IssuableFinder do
context ':projects option' do
let(:query) do
{ issuable_type: issuable_type }
{ issuable_type: issuable_type, issuable_state: issuable_state }
end
before do
......@@ -286,16 +328,7 @@ RSpec.describe Gitlab::Insights::Finders::IssuableFinder do
include_examples "insights issuable finder" do
let(:issuable_type) { 'merge_request' }
let(:project_association_key) { :source_project }
let(:extra_issuable_attrs) do
[
{ source_branch: "add_images_and_changes" },
{ source_branch: "improve/awesome" },
{ source_branch: "feature_conflict" },
{ source_branch: "markdown" },
{ source_branch: "feature_one" },
{ source_branch: "merged-target" }
]
end
let(:extra_issuable_attrs) { merge_request_extra_attrs }
end
end
end
......@@ -332,17 +365,17 @@ RSpec.describe Gitlab::Insights::Finders::IssuableFinder do
include_examples "insights issuable finder" do
let(:issuable_type) { 'merge_request' }
let(:project_association_key) { :source_project }
let(:extra_issuable_attrs) do
[
{ source_branch: "add_images_and_changes" },
{ source_branch: "improve/awesome" },
{ source_branch: "feature_conflict" },
{ source_branch: "markdown" },
{ source_branch: "feature_one" },
{ source_branch: "merged-target" }
]
let(:extra_issuable_attrs) { merge_request_extra_attrs }
end
end
context 'merged merge requests' do
include_examples "insights issuable finder" do
let(:issuable_state) { :merged }
let(:issuable_type) { 'merge_request' }
let(:project_association_key) { :source_project }
let(:extra_issuable_attrs) { merge_request_extra_attrs }
end
end
end
end
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Gitlab::Insights::Reducers::BaseReducer do
let(:issuable_relation) { Gitlab::Insights::Finders::IssuableFinder.new(project, nil, query: query).find }
let(:project) { create(:project, :public) }
let(:query) do
{
issuable_type: 'issue'
}
end
it 'raises NotImplementedError' do
expect { described_class.reduce(issuable_relation, period: query[:group_by]) }.to raise_error(NotImplementedError)
end
describe '#issuable_type' do
context 'with issues' do
it 'returns :issue' do
expect(described_class.__send__(:new, issuable_relation).issuable_type).to eq(:issue)
end
end
context 'with merge requests' do
let(:query) do
{
issuable_type: 'merge_request'
}
end
it 'returns :merge_request' do
expect(described_class.__send__(:new, issuable_relation).issuable_type).to eq(:merge_request)
end
end
end
end
......@@ -3,7 +3,7 @@
require 'spec_helper'
RSpec.describe Gitlab::Insights::Reducers::CountPerLabelReducer do
include_context 'Insights reducers context'
include_context 'Insights issues reducer context'
def find_issuables(project, query)
Gitlab::Insights::Finders::IssuableFinder.new(project, nil, query: query).find
......
......@@ -3,49 +3,79 @@
require 'spec_helper'
RSpec.describe Gitlab::Insights::Reducers::CountPerPeriodReducer do
include_context 'Insights reducers context'
def find_issuables(project, query)
Gitlab::Insights::Finders::IssuableFinder.new(project, nil, query: query).find
end
def reduce(issuable_relation, period, period_limit = 5, period_field = :created_at)
def reduce(issuable_relation, period:, period_limit: 5, period_field: :created_at)
described_class.reduce(issuable_relation, period: period, period_limit: period_limit, period_field: period_field)
end
let(:issuable_relation) { find_issuables(project, query) }
let(:expected) do
{
'January 2019' => 1,
'February 2019' => 0,
'March 2019' => 1,
'April 2019' => 1,
'May 2019' => 0
}
end
subject { reduce(issuable_relation, period: query[:group_by]) }
context 'with no issues' do
around do |example|
Timecop.freeze(Time.utc(2019, 5, 5)) { example.run }
end
let(:project) { create(:project, :public) }
let(:query) do
{
state: 'opened',
issuable_state: 'opened',
issuable_type: 'issue',
filter_labels: [label_bug.title],
group_by: 'month',
period_limit: 5
}
end
let(:issuable_relation) { find_issuables(project, query) }
subject { reduce(issuable_relation, query[:group_by]) }
let(:expected) do
{
'January 2019' => 1,
'January 2019' => 0,
'February 2019' => 0,
'March 2019' => 1,
'April 2019' => 1,
'March 2019' => 0,
'April 2019' => 0,
'May 2019' => 0
}
end
it 'returns no issuables' do
expect(reduce(issuable_relation, period: query[:group_by])).to eq(expected)
end
end
context 'with open issues' do
include_context 'Insights issues reducer context', :opened
let(:query) do
{
issuable_state: 'opened',
issuable_type: 'issue',
filter_labels: [label_bug.title],
group_by: 'month',
period_limit: 5
}
end
it 'raises an error for an unknown :period option' do
expect { reduce(issuable_relation, 'unknown') }.to raise_error(described_class::InvalidPeriodError, "Invalid value for `period`: `unknown`. Allowed values are #{described_class::VALID_PERIOD}!")
expect { reduce(issuable_relation, period: 'unknown') }.to raise_error(described_class::InvalidPeriodError, "Invalid value for `period`: `unknown`. Allowed values are #{described_class::VALID_PERIOD}!")
end
it 'raises an error for an unknown :period_field option' do
expect { reduce(issuable_relation, 'month', 5, :foo) }.to raise_error(described_class::InvalidPeriodFieldError, "Invalid value for `period_field`: `foo`. Allowed values are #{described_class::VALID_PERIOD_FIELD}!")
expect { reduce(issuable_relation, period: 'month', period_limit: 5, period_field: :foo) }.to raise_error(described_class::InvalidPeriodFieldError, "Invalid value for `period_field`: `foo`. Allowed values are #{described_class::VALID_PERIOD_FIELDS[:issue]}!")
end
it 'raises an error for an unknown :period_limit option' do
expect { reduce(issuable_relation, 'month', -1) }.to raise_error(described_class::InvalidPeriodLimitError, "Invalid value for `period_limit`: `-1`. Value must be greater than 0!")
expect { reduce(issuable_relation, period: 'month', period_limit: -1) }.to raise_error(described_class::InvalidPeriodLimitError, "Invalid value for `period_limit`: `-1`. Value must be greater than 0!")
end
it 'returns issuables with only the needed fields' do
......@@ -56,6 +86,97 @@ RSpec.describe Gitlab::Insights::Reducers::CountPerPeriodReducer do
control_queries = ActiveRecord::QueryRecorder.new { subject }
create(:labeled_issue, :opened, created_at: Time.utc(2019, 2, 5), labels: [label_bug], project: project)
expect { reduce(find_issuables(project, query), query[:group_by]) }.not_to exceed_query_limit(control_queries)
expect { reduce(find_issuables(project, query), period: query[:group_by]) }.not_to exceed_query_limit(control_queries)
end
end
context 'with closed issues' do
include_context 'Insights issues reducer context', :closed
let(:query) do
{
issuable_state: 'closed',
issuable_type: 'issue',
filter_labels: [label_bug.title],
group_by: 'month',
period_limit: 5
}
end
it 'returns issuables with only the needed fields' do
expect(reduce(issuable_relation, period: query[:group_by], period_field: :closed_at)).to eq(expected)
end
end
context 'with opened merge requests' do
include_context 'Insights merge requests reducer context', :opened
let(:query) do
{
issuable_state: 'opened',
issuable_type: 'merge_request',
filter_labels: [label_bug.title],
group_by: 'month',
period_limit: 5
}
end
it 'raises an error for an unknown :period_field option' do
expect { reduce(issuable_relation, period: 'month', period_limit: 5, period_field: :foo) }.to raise_error(described_class::InvalidPeriodFieldError, "Invalid value for `period_field`: `foo`. Allowed values are #{described_class::VALID_PERIOD_FIELDS[:merge_request]}!")
end
it 'returns issuables with only the needed fields' do
expect(reduce(issuable_relation, period: query[:group_by], period_field: :created_at)).to eq(expected)
end
end
context 'with merged merge requests' do
include_context 'Insights merge requests reducer context', :merged
# Populate the MR metrics' merged_at
before do
(0..3).each do |i|
merge_request = public_send("issuable#{i}")
merge_request_metrics_service = MergeRequestMetricsService.new(merge_request.metrics)
Event.transaction do
Timecop.freeze(merge_request.created_at) do
merge_event = EventCreateService.new.merge_mr(merge_request, merge_request.author)
merge_request_metrics_service.merge(merge_event)
end
end
end
end
let(:query) do
{
issuable_state: 'merged',
issuable_type: 'merge_request',
filter_labels: [label_bug.title],
group_by: 'month',
period_limit: 5
}
end
it 'returns issuables with only the needed fields' do
expect(reduce(issuable_relation, period: query[:group_by], period_field: :merged_at)).to eq(expected)
end
end
context 'with closed merge requests' do
include_context 'Insights merge requests reducer context', :closed
let(:query) do
{
issuable_state: 'closed',
issuable_type: 'merge_request',
filter_labels: [label_bug.title],
group_by: 'month',
period_limit: 5
}
end
it 'returns issuables with only the needed fields' do
expect(reduce(issuable_relation, period: query[:group_by], period_field: :created_at)).to eq(expected)
end
end
end
......@@ -3,7 +3,7 @@
require 'spec_helper'
RSpec.describe Gitlab::Insights::Reducers::LabelCountPerPeriodReducer do
include_context 'Insights reducers context'
include_context 'Insights issues reducer context'
def find_issuables(project, query)
Gitlab::Insights::Finders::IssuableFinder.new(project, nil, query: query).find
......
# frozen_string_literal: true
RSpec.shared_context 'Insights reducers context' do
RSpec.shared_context 'Insights issues reducer context' do |state = :opened|
around do |example|
Timecop.freeze(Time.utc(2019, 5, 5)) { example.run }
end
let(:period_fied) do
case state
when :closed
:closed_at
else
:created_at
end
end
let(:project) { create(:project, :public) }
let(:label_bug) { create(:label, project: project, name: 'Bug') }
let(:label_manage) { create(:label, project: project, name: 'Manage') }
let(:label_plan) { create(:label, project: project, name: 'Plan') }
let!(:issuable0) { create(:labeled_issue, state, period_fied => Time.utc(2019, 1, 5), project: project) }
let!(:issuable1) { create(:labeled_issue, state, period_fied => Time.utc(2019, 1, 5), labels: [label_bug], project: project) }
let!(:issuable2) { create(:labeled_issue, state, period_fied => Time.utc(2019, 3, 5), labels: [label_bug, label_manage, label_plan], project: project) }
let!(:issuable3) { create(:labeled_issue, state, period_fied => Time.utc(2019, 4, 5), labels: [label_bug, label_plan], project: project) }
end
RSpec.shared_context 'Insights merge requests reducer context' do |state = :opened|
around do |example|
Timecop.freeze(Time.utc(2019, 5, 5)) { example.run }
end
......@@ -9,8 +33,8 @@ RSpec.shared_context 'Insights reducers context' do
let(:label_bug) { create(:label, project: project, name: 'Bug') }
let(:label_manage) { create(:label, project: project, name: 'Manage') }
let(:label_plan) { create(:label, project: project, name: 'Plan') }
let!(:issuable0) { create(:labeled_issue, :opened, created_at: Time.utc(2019, 1, 5), project: project) }
let!(:issuable1) { create(:labeled_issue, :opened, created_at: Time.utc(2019, 1, 5), labels: [label_bug], project: project) }
let!(:issuable2) { create(:labeled_issue, :opened, created_at: Time.utc(2019, 3, 5), labels: [label_bug, label_manage, label_plan], project: project) }
let!(:issuable3) { create(:labeled_issue, :opened, created_at: Time.utc(2019, 4, 5), labels: [label_bug, label_plan], project: project) }
let!(:issuable0) { create(:labeled_merge_request, state, :simple, created_at: Time.utc(2019, 1, 5), source_project: project) }
let!(:issuable1) { create(:labeled_merge_request, state, :with_image_diffs, created_at: Time.utc(2019, 1, 5), labels: [label_bug], source_project: project) }
let!(:issuable2) { create(:labeled_merge_request, state, :without_diffs, created_at: Time.utc(2019, 3, 5), labels: [label_bug, label_manage, label_plan], source_project: project) }
let!(:issuable3) { create(:labeled_merge_request, state, :rebased, created_at: Time.utc(2019, 4, 5), labels: [label_bug, label_plan], source_project: project) }
end
......@@ -29,6 +29,7 @@ module Quality
assignee_ids: Array(team.pluck(:id).sample(3)),
labels: labels.join(',')
}
params[:closed_at] = params[:created_at] + rand(35).days if params[:state] == 'closed'
issue = ::Issues::CreateService.new(project, team.sample, params).execute
if issue.persisted?
......
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