Коммит dd38e289 создал по автору Jarka Košanová's avatar Jarka Košanová
Просмотр файлов

Extract issue trackers data fields 

- move issue trackers properties to
a separated table
- keep backwards compatibily and still be able to read
from properties
владелец 25cb337c
Скрыть пробелы
Построчно Рядом
app/models/project_services/bugzilla_service.rb
Просмотр файла @ dd38e289
......@@ -3,8 +3,6 @@
class BugzillaService < IssueTrackerService
validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
prop_accessor :project_url, :issues_url, :new_issue_url
def default_title
'Bugzilla'
end
......
app/models/project_services/custom_issue_tracker_service.rb
Просмотр файла @ dd38e289
......@@ -3,8 +3,6 @@
class CustomIssueTrackerService < IssueTrackerService
validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
prop_accessor :title, :description, :project_url, :issues_url, :new_issue_url
def default_title
'Custom Issue Tracker'
end
......
app/models/project_services/data_fields.rb
Просмотр файла @ dd38e289
......@@ -3,8 +3,56 @@
module DataFields
extend ActiveSupport::Concern
class_methods do
# Provide convenient accessor methods for data fields.
# TODO: Simplify as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/63084
def data_field(*args)
args.each do |arg|
self.class_eval <<-RUBY, __FILE__, __LINE__ + 1
unless method_defined?(arg)
def #{arg}
data_fields.send('#{arg}') || (properties && properties['#{arg}'])
end
end
def #{arg}=(value)
@old_data_fields ||= {}
@old_data_fields['#{arg}'] ||= #{arg} # set only on the first assignment, IOW we remember the original value only
data_fields.send('#{arg}=', value)
end
def #{arg}_touched?
@old_data_fields ||= {}
@old_data_fields.has_key?('#{arg}')
end
def #{arg}_changed?
#{arg}_touched? && @old_data_fields['#{arg}'] != #{arg}
end
def #{arg}_was
return unless #{arg}_touched?
return if data_fields.persisted? # arg_was does not work for attr_encrypted
legacy_properties_data['#{arg}']
end
RUBY
end
end
end
included do
has_one :issue_tracker_data
has_one :jira_tracker_data
has_one :issue_tracker_data, autosave: true
has_one :jira_tracker_data, autosave: true
def data_fields
raise NotImplementedError
end
def data_fields_present?
data_fields.persisted?
rescue NotImplementedError
false
end
end
end
app/models/project_services/gitlab_issue_tracker_service.rb
Просмотр файла @ dd38e289
......@@ -5,8 +5,6 @@ class GitlabIssueTrackerService < IssueTrackerService
validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
prop_accessor :project_url, :issues_url, :new_issue_url
default_value_for :default, true
def default_title
......
app/models/project_services/issue_tracker_data.rb
Просмотр файла @ dd38e289
......@@ -6,9 +6,6 @@ class IssueTrackerData < ApplicationRecord
delegate :activated?, to: :service, allow_nil: true
validates :service, presence: true
validates :project_url, presence: true, public_url: { enforce_sanitization: true }, if: :activated?
validates :issues_url, presence: true, public_url: { enforce_sanitization: true }, if: :activated?
validates :new_issue_url, public_url: { enforce_sanitization: true }, if: :activated?
def self.encryption_options
{
......
app/models/project_services/issue_tracker_service.rb
Просмотр файла @ dd38e289
......@@ -3,9 +3,14 @@
class IssueTrackerService < Service
validate :one_issue_tracker, if: :activated?, on: :manual_change
# TODO: we can probably just delegate as part of
# https://gitlab.com/gitlab-org/gitlab-ce/issues/63084
data_field :project_url, :issues_url, :new_issue_url
default_value_for :category, 'issue_tracker'
before_save :handle_properties
before_validation :handle_properties
before_validation :set_default_data, on: :create
# Pattern used to extract links from comments
# Override this method on services that uses different patterns
......@@ -43,12 +48,31 @@ def description
end
def handle_properties
properties.slice('title', 'description').each do |key, _|
# this has been moved from initialize_properties and should be improved
# as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/63084
return unless properties
@legacy_properties_data = properties.dup
data_values = properties.slice!('title', 'description')
properties.each do |key, _|
current_value = self.properties.delete(key)
value = attribute_changed?(key) ? attribute_change(key).last : current_value
write_attribute(key, value)
end
data_values.reject! { |key| data_fields.changed.include?(key) }
data_fields.assign_attributes(data_values) if data_values.present?
self.properties = {}
end
def legacy_properties_data
@legacy_properties_data ||= {}
end
def data_fields
issue_tracker_data || self.build_issue_tracker_data
end
def default?
......@@ -56,7 +80,7 @@ def default?
end
def issue_url(iid)
self.issues_url.gsub(':id', iid.to_s)
issues_url.gsub(':id', iid.to_s)
end
def issue_tracker_path
......@@ -80,25 +104,22 @@ def fields
]
end
def initialize_properties
{}
end
# Initialize with default properties values
# or receive a block with custom properties
def initialize_properties(&block)
return unless properties.nil?
if enabled_in_gitlab_config
if block_given?
yield
else
self.properties = {
title: issues_tracker['title'],
project_url: issues_tracker['project_url'],
issues_url: issues_tracker['issues_url'],
new_issue_url: issues_tracker['new_issue_url']
}
end
else
self.properties = {}
end
def set_default_data
return unless issues_tracker.present?
self.title ||= issues_tracker['title']
# we don't want to override if we have set something
return if project_url || issues_url || new_issue_url
data_fields.project_url = issues_tracker['project_url']
data_fields.issues_url = issues_tracker['issues_url']
data_fields.new_issue_url = issues_tracker['new_issue_url']
end
def self.supported_events
......
app/models/project_services/jira_service.rb
Просмотр файла @ dd38e289
......@@ -17,7 +17,10 @@ class JiraService < IssueTrackerService
# Jira Cloud version is deprecating authentication via username and password.
# We should use username/password for Jira Server and email/api_token for Jira Cloud,
# for more information check: https://gitlab.com/gitlab-org/gitlab-ce/issues/49936.
prop_accessor :username, :password, :url, :api_url, :jira_issue_transition_id
# TODO: we can probably just delegate as part of
# https://gitlab.com/gitlab-org/gitlab-ce/issues/63084
data_field :username, :password, :url, :api_url, :jira_issue_transition_id
before_update :reset_password
......@@ -35,24 +38,34 @@ def self.reference_pattern(only_long: true)
end
def initialize_properties
super do
self.properties = {
url: issues_tracker['url'],
api_url: issues_tracker['api_url']
}
end
{}
end
def data_fields
jira_tracker_data || self.build_jira_tracker_data
end
def reset_password
self.password = nil if reset_password?
data_fields.password = nil if reset_password?
end
def set_default_data
return unless issues_tracker.present?
self.title ||= issues_tracker['title']
return if url
data_fields.url ||= issues_tracker['url']
data_fields.api_url ||= issues_tracker['api_url']
end
def options
url = URI.parse(client_url)
{
username: self.username,
password: self.password,
username: username,
password: password,
site: URI.join(url, '/').to_s, # Intended to find the root
context_path: url.path,
auth_type: :basic,
......
app/models/project_services/jira_tracker_data.rb
Просмотр файла @ dd38e289
......@@ -6,13 +6,6 @@ class JiraTrackerData < ApplicationRecord
delegate :activated?, to: :service, allow_nil: true
validates :service, presence: true
validates :url, public_url: { enforce_sanitization: true }, presence: true, if: :activated?
validates :api_url, public_url: { enforce_sanitization: true }, allow_blank: true
validates :username, presence: true, if: :activated?
validates :password, presence: true, if: :activated?
validates :jira_issue_transition_id,
format: { with: Gitlab::Regex.jira_transition_id_regex, message: s_("JiraService|transition ids can have only numbers which can be split with , or ;") },
allow_blank: true
def self.encryption_options
{
......
app/models/project_services/redmine_service.rb
Просмотр файла @ dd38e289
......@@ -3,8 +3,6 @@
class RedmineService < IssueTrackerService
validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
prop_accessor :project_url, :issues_url, :new_issue_url
def default_title
'Redmine'
end
......
app/models/project_services/youtrack_service.rb
Просмотр файла @ dd38e289
......@@ -3,8 +3,6 @@
class YoutrackService < IssueTrackerService
validates :project_url, :issues_url, presence: true, public_url: true, if: :activated?
prop_accessor :project_url, :issues_url
# {PROJECT-KEY}-{NUMBER} Examples: YT-1, PRJ-1, gl-030
def self.reference_pattern(only_long: false)
if only_long
......
lib/api/entities.rb
Просмотр файла @ dd38e289
......@@ -1044,7 +1044,12 @@ class ProjectService < Grape::Entity
expose :job_events
# Expose serialized properties
expose :properties do |service, options|
service.properties.slice(*service.api_field_names)
# TODO: Simplify as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/63084
if service.data_fields_present?
service.data_fields.as_json.slice(*service.api_field_names)
else
service.properties.slice(*service.api_field_names)
end
end
end
......
lib/gitlab/usage_data.rb
Просмотр файла @ dd38e289
......@@ -3,6 +3,7 @@
module Gitlab
class UsageData
APPROXIMATE_COUNT_MODELS = [Label, MergeRequest, Note, Todo].freeze
BATCH_SIZE = 100
class << self
def data(force_refresh: false)
......@@ -13,10 +14,10 @@ def data(force_refresh: false)
def uncached_data
license_usage_data.merge(system_usage_data)
.merge(features_usage_data)
.merge(components_usage_data)
.merge(cycle_analytics_usage_data)
.merge(usage_counters)
.merge(features_usage_data)
.merge(components_usage_data)
.merge(cycle_analytics_usage_data)
.merge(usage_counters)
end
def to_json(force_refresh: false)
......@@ -96,9 +97,8 @@ def system_usage_data
todos: count(Todo),
uploads: count(Upload),
web_hooks: count(WebHook)
}
.merge(services_usage)
.merge(approximate_counts)
}.merge(services_usage)
.merge(approximate_counts)
}.tap do |data|
data[:counts][:user_preferences] = user_preferences_usage
end
......@@ -173,17 +173,34 @@ def services_usage
def jira_usage
# Jira Cloud does not support custom domains as per https://jira.atlassian.com/browse/CLOUD-6999
# so we can just check for subdomains of atlassian.net
services = count(
Service.unscoped.where(type: :JiraService, active: true)
.group("CASE WHEN properties LIKE '%.atlassian.net%' THEN 'cloud' ELSE 'server' END"),
fallback: Hash.new(-1)
)
{
projects_jira_server_active: services['server'] || 0,
projects_jira_cloud_active: services['cloud'] || 0,
projects_jira_active: services['server'] == -1 ? -1 : services.values.sum
results = {
projects_jira_server_active: 0,
projects_jira_cloud_active: 0,
projects_jira_active: -1
}
Service.unscoped
.where(type: :JiraService, active: true)
.includes(:jira_tracker_data)
.find_in_batches(batch_size: BATCH_SIZE) do |services|
counts = services.group_by do |service|
# TODO: Simplify as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/63084
service_url = service.data_fields&.url || (service.properties && service.properties['url'])
service_url&.include?('.atlassian.net') ? :cloud : :server
end
results[:projects_jira_server_active] += counts[:server].count if counts[:server]
results[:projects_jira_cloud_active] += counts[:cloud].count if counts[:cloud]
if results[:projects_jira_active] == -1
results[:projects_jira_active] = count(services)
else
results[:projects_jira_active] += count(services)
end
end
results
end
def user_preferences_usage
......
spec/factories/services.rb
Просмотр файла @ dd38e289
......@@ -9,11 +9,7 @@
factory :custom_issue_tracker_service, class: CustomIssueTrackerService do
project
active true
properties(
project_url: 'https://project.url.com',
issues_url: 'https://issues.url.com',
new_issue_url: 'https://newissue.url.com'
)
issue_tracker
end
factory :emails_on_push_service do
......@@ -47,12 +43,24 @@
factory :jira_service do
project
active true
properties(
url: 'https://jira.example.com',
username: 'jira_user',
password: 'my-secret-password',
project_key: 'jira-key'
)
transient do
create_data true
url 'https://jira.example.com'
api_url nil
username 'jira_username'
password 'jira_password'
jira_issue_transition_id '56-1'
end
after(:build) do |service, evaluator|
if evaluator.create_data
create(:jira_tracker_data, service: service,
url: evaluator.url, api_url: evaluator.api_url, jira_issue_transition_id: evaluator.jira_issue_transition_id,
username: evaluator.username, password: evaluator.password
)
end
end
end
factory :bugzilla_service do
......@@ -80,20 +88,26 @@
end
trait :issue_tracker do
properties(
project_url: 'http://issue-tracker.example.com',
issues_url: 'http://issue-tracker.example.com/issues/:id',
new_issue_url: 'http://issue-tracker.example.com'
)
transient do
create_data true
project_url 'http://issuetracker.example.com'
issues_url 'http://issues.example.com/issues/:id'
new_issue_url 'http://new-issue.example.com'
end
after(:build) do |service, evaluator|
if evaluator.create_data
create(:issue_tracker_data, service: service,
project_url: evaluator.project_url, issues_url: evaluator.issues_url, new_issue_url: evaluator.new_issue_url
)
end
end
end
trait :jira_cloud_service do
properties(
url: 'https://mysite.atlassian.net',
username: 'jira_user',
password: 'my-secret-password',
project_key: 'jira-key'
)
url 'https://mysite.atlassian.net'
username 'jira_user'
password 'my-secret-password'
end
factory :hipchat_service do
......@@ -102,15 +116,21 @@
token 'test_token'
end
# this is for testing storing values inside properties, which is deprecated and will be removed in
# https://gitlab.com/gitlab-org/gitlab-ce/issues/63084
trait :without_properties_callback do
jira_tracker_data nil
issue_tracker_data nil
create_data false
after(:build) do |service|
allow(service).to receive(:handle_properties)
IssueTrackerService.skip_callback(:validation, :before, :handle_properties)
end
after(:create) do |service|
# we have to remove the stub because the behaviour of
# handle_properties method is tested after the creation
allow(service).to receive(:handle_properties).and_call_original
to_create { |instance| instance.save(validate: false)}
after(:create) do
IssueTrackerService.set_callback(:validation, :before, :handle_properties)
end
end
end
spec/lib/banzai/pipeline/gfm_pipeline_spec.rb
Просмотр файла @ dd38e289
......@@ -34,7 +34,7 @@
result = described_class.call(markdown, project: project)[:output]
link = result.css('a').first
expect(link['href']).to eq 'http://issue-tracker.example.com/issues/12'
expect(link['href']).to eq 'http://issues.example.com/issues/12'
end
it 'parses cross-project references to regular issues' do
......@@ -63,7 +63,7 @@
result = described_class.call(markdown, project: project)[:output]
link = result.css('a').first
expect(link['href']).to eq 'http://issue-tracker.example.com/issues/12'
expect(link['href']).to eq 'http://issues.example.com/issues/12'
end
it 'allows to use long external reference syntax for Redmine' do
......@@ -72,7 +72,7 @@
result = described_class.call(markdown, project: project)[:output]
link = result.css('a').first
expect(link['href']).to eq 'http://issue-tracker.example.com/issues/12'
expect(link['href']).to eq 'http://issues.example.com/issues/12'
end
it 'parses cross-project references to regular issues' do
......
spec/lib/gitlab/usage_data_spec.rb
Просмотр файла @ dd38e289
......@@ -3,14 +3,16 @@
require 'spec_helper'
describe Gitlab::UsageData do
let(:projects) { create_list(:project, 3) }
let(:projects) { create_list(:project, 4) }
let!(:board) { create(:board, project: projects[0]) }
describe '#data' do
before do
create(:jira_service, project: projects[0])
create(:jira_service, project: projects[1])
create(:jira_service, :without_properties_callback, project: projects[1])
create(:jira_service, :jira_cloud_service, project: projects[2])
create(:jira_service, :without_properties_callback, project: projects[3],
properties: { url: 'https://mysite.atlassian.net' })
create(:prometheus_service, project: projects[1])
create(:service, project: projects[0], type: 'SlackSlashCommandsService', active: true)
create(:service, project: projects[1], type: 'SlackService', active: true)
......@@ -156,7 +158,7 @@
count_data = subject[:counts]
expect(count_data[:boards]).to eq(1)
expect(count_data[:projects]).to eq(3)
expect(count_data[:projects]).to eq(4)
expect(count_data.keys).to include(*expected_keys)
expect(expected_keys - count_data.keys).to be_empty
end
......@@ -164,14 +166,14 @@
it 'gathers projects data correctly' do
count_data = subject[:counts]
expect(count_data[:projects]).to eq(3)
expect(count_data[:projects]).to eq(4)
expect(count_data[:projects_prometheus_active]).to eq(1)
expect(count_data[:projects_jira_active]).to eq(3)
expect(count_data[:projects_jira_active]).to eq(4)
expect(count_data[:projects_jira_server_active]).to eq(2)
expect(count_data[:projects_jira_cloud_active]).to eq(1)
expect(count_data[:projects_jira_cloud_active]).to eq(2)
expect(count_data[:projects_slack_notifications_active]).to eq(2)
expect(count_data[:projects_slack_slash_active]).to eq(1)
expect(count_data[:projects_with_repositories_enabled]).to eq(2)
expect(count_data[:projects_with_repositories_enabled]).to eq(3)
expect(count_data[:projects_with_error_tracking_enabled]).to eq(1)
expect(count_data[:clusters_enabled]).to eq(7)
......
spec/models/project_services/bugzilla_service_spec.rb
Просмотр файла @ dd38e289
......@@ -48,7 +48,7 @@
create(:bugzilla_service, :without_properties_callback, properties: properties)
end
include_examples 'issue tracker fields'
it_behaves_like 'issue tracker fields'
end
context 'when data are stored in separated fields' do
......@@ -56,7 +56,7 @@
create(:bugzilla_service, title: title, description: description, properties: access_params)
end
include_examples 'issue tracker fields'
it_behaves_like 'issue tracker fields'
end
context 'when data are stored in both properties and separated fields' do
......@@ -65,7 +65,7 @@
create(:bugzilla_service, :without_properties_callback, title: title, description: description, properties: properties)
end
include_examples 'issue tracker fields'
it_behaves_like 'issue tracker fields'
end
context 'when no title & description are set' do
......
spec/models/project_services/custom_issue_tracker_service_spec.rb
Просмотр файла @ dd38e289
......@@ -62,7 +62,7 @@
create(:custom_issue_tracker_service, :without_properties_callback, properties: properties)
end
include_examples 'issue tracker fields'
it_behaves_like 'issue tracker fields'
end
context 'when data are stored in separated fields' do
......@@ -70,7 +70,7 @@
create(:custom_issue_tracker_service, title: title, description: description, properties: access_params)
end
include_examples 'issue tracker fields'
it_behaves_like 'issue tracker fields'
end
context 'when data are stored in both properties and separated fields' do
......@@ -79,7 +79,7 @@
create(:custom_issue_tracker_service, :without_properties_callback, title: title, description: description, properties: properties)
end
include_examples 'issue tracker fields'
it_behaves_like 'issue tracker fields'
end
context 'when no title & description are set' do
......
spec/models/project_services/data_fields_spec.rb 0 → 100644
Просмотр файла @ dd38e289
# frozen_string_literal: true
require 'spec_helper'
describe DataFields do
let(:url) { 'http://url.com' }
let(:username) { 'username_one' }
let(:properties) do
{ url: url, username: username }
end
shared_examples 'data fields' do
describe '#arg' do
it 'returns an argument correctly' do
expect(service.url).to eq(url)
end
end
describe '{arg}_changed?' do
it 'returns false when the property has not been assigned a new value' do
service.username = 'new_username'
service.validate
expect(service.url_changed?).to be_falsy
end
it 'returns true when the property has been assigned a different value' do
service.url = "http://example.com"
service.validate
expect(service.url_changed?).to be_truthy
end
it 'returns true when the property has been assigned a different value twice' do
service.url = "http://example.com"
service.url = "http://example.com"
service.validate
expect(service.url_changed?).to be_truthy
end
it 'returns false when the property has been re-assigned the same value' do
service.url = 'http://url.com'
service.validate
expect(service.url_changed?).to be_falsy
end
end
describe '{arg}_touched?' do
it 'returns false when the property has not been assigned a new value' do
service.username = 'new_username'
service.validate
expect(service.url_changed?).to be_falsy
end
it 'returns true when the property has been assigned a different value' do
service.url = "http://example.com"
service.validate
expect(service.url_changed?).to be_truthy
end
it 'returns true when the property has been assigned a different value twice' do
service.url = "http://example.com"
service.url = "http://example.com"
service.validate
expect(service.url_changed?).to be_truthy
end
it 'returns true when the property has been re-assigned the same value' do
service.url = 'http://url.com'
expect(service.url_touched?).to be_truthy
end
it 'returns false when the property has been re-assigned the same value' do
service.url = 'http://url.com'
service.validate
expect(service.url_changed?).to be_falsy
end
end
end
context 'when data are stored in data_fields' do
let(:service) do
create(:jira_service, url: url, username: username)
end
it_behaves_like 'data fields'
describe '{arg}_was?' do
it 'returns nil' do
service.url = 'http://example.com'
service.validate
expect(service.url_was).to be_nil
end
end
end
context 'when data are stored in properties' do
let(:service) { create(:jira_service, :without_properties_callback, properties: properties) }
it_behaves_like 'data fields'
describe '{arg}_was?' do
it 'returns nil when the property has not been assigned a new value' do
service.username = 'new_username'
service.validate
expect(service.url_was).to be_nil
end
it 'returns initial value when the property has been assigned a different value' do
service.url = 'http://example.com'
service.validate
expect(service.url_was).to eq('http://url.com')
end
it 'returns initial value when the property has been re-assigned the same value' do
service.url = 'http://url.com'
service.validate
expect(service.url_was).to eq('http://url.com')
end
end
end
context 'when data are stored in both properties and data_fields' do
let(:service) do
create(:jira_service, :without_properties_callback, active: false, properties: properties).tap do |service|
create(:jira_tracker_data, properties.merge(service: service))
end
end
it_behaves_like 'data fields'
describe '{arg}_was?' do
it 'returns nil' do
service.url = 'http://example.com'
service.validate
expect(service.url_was).to be_nil
end
end
end
end
spec/models/project_services/gitlab_issue_tracker_service_spec.rb
Просмотр файла @ dd38e289
......@@ -65,7 +65,7 @@
create(:gitlab_issue_tracker_service, :without_properties_callback, properties: properties)
end
include_examples 'issue tracker fields'
it_behaves_like 'issue tracker fields'
end
context 'when data are stored in separated fields' do
......@@ -73,7 +73,7 @@
create(:gitlab_issue_tracker_service, title: title, description: description, properties: access_params)
end
include_examples 'issue tracker fields'
it_behaves_like 'issue tracker fields'
end
context 'when data are stored in both properties and separated fields' do
......@@ -82,7 +82,7 @@
create(:gitlab_issue_tracker_service, :without_properties_callback, title: title, description: description, properties: properties)
end
include_examples 'issue tracker fields'
it_behaves_like 'issue tracker fields'
end
context 'when no title & description are set' do
......
spec/models/project_services/issue_tracker_data_spec.rb
Просмотр файла @ dd38e289
......@@ -8,28 +8,4 @@
describe 'Associations' do
it { is_expected.to belong_to :service }
end
describe 'Validations' do
subject { described_class.new(service: service) }
context 'url validations' do
context 'when service is inactive' do
it { is_expected.not_to validate_presence_of(:project_url) }
it { is_expected.not_to validate_presence_of(:issues_url) }
end
context 'when service is active' do
before do
service.update(active: true)
end
it_behaves_like 'issue tracker service URL attribute', :project_url
it_behaves_like 'issue tracker service URL attribute', :issues_url
it_behaves_like 'issue tracker service URL attribute', :new_issue_url
it { is_expected.to validate_presence_of(:project_url) }
it { is_expected.to validate_presence_of(:issues_url) }
end
end
end
end
Поддерживает Markdown
0% или .
You are about to add 0 people to the discussion. Proceed with caution.
Сначала завершите редактирование этого сообщения!
Пожалуйста, зарегистрируйтесь или чтобы прокомментировать