From 20794440918b5435bd86838252e387cbea345812 Mon Sep 17 00:00:00 2001 From: Reuben Pereira Date: Mon, 4 Feb 2019 12:12:24 +0000 Subject: DB and model changes for Sentry project selection dropdown --- .../project_error_tracking_setting.rb | 83 +++++++++- ..._add_columns_project_error_tracking_settings.rb | 16 ++ db/schema.rb | 6 +- lib/gitlab/import_export/import_export.yml | 1 + spec/factories/project_error_tracking_settings.rb | 2 + .../gitlab/import_export/safe_model_attributes.yml | 3 +- .../project_error_tracking_setting_spec.rb | 176 ++++++++++++++++++++- 7 files changed, 278 insertions(+), 9 deletions(-) create mode 100644 db/migrate/20190115092821_add_columns_project_error_tracking_settings.rb diff --git a/app/models/error_tracking/project_error_tracking_setting.rb b/app/models/error_tracking/project_error_tracking_setting.rb index 7f4947ba27a..31084c54bdc 100644 --- a/app/models/error_tracking/project_error_tracking_setting.rb +++ b/app/models/error_tracking/project_error_tracking_setting.rb @@ -8,9 +8,13 @@ module ErrorTracking belongs_to :project - validates :api_url, length: { maximum: 255 }, public_url: true, url: { enforce_sanitization: true } + validates :api_url, length: { maximum: 255 }, public_url: true, url: { enforce_sanitization: true, ascii_only: true }, allow_nil: true - validate :validate_api_url_path + validates :api_url, presence: true, if: :enabled + + validate :validate_api_url_path, if: :enabled + + validates :token, presence: true, if: :enabled attr_encrypted :token, mode: :per_attribute_iv, @@ -19,6 +23,31 @@ module ErrorTracking after_save :clear_reactive_cache! + def project_name + super || project_name_from_slug + end + + def organization_name + super || organization_name_from_slug + end + + def project_slug + project_slug_from_api_url + end + + def organization_slug + organization_slug_from_api_url + end + + def self.build_api_url_from(api_host:, project_slug:, organization_slug:) + uri = Addressable::URI.parse("#{api_host}/api/0/projects/#{organization_slug}/#{project_slug}/") + uri.path = uri.path.squeeze('/') + + uri.to_s + rescue Addressable::URI::InvalidURIError + api_host + end + def sentry_client Sentry::Client.new(api_url, token) end @@ -33,6 +62,10 @@ module ErrorTracking end end + def list_sentry_projects + { projects: sentry_client.list_projects } + end + def calculate_reactive_cache(request, opts) case request when 'list_issues' @@ -47,13 +80,53 @@ module ErrorTracking url.sub('api/0/projects/', '') end + def api_host + return if api_url.blank? + + # This returns http://example.com/ + Addressable::URI.join(api_url, '/').to_s + end + private + def project_name_from_slug + @project_name_from_slug ||= project_slug_from_api_url&.titleize + end + + def organization_name_from_slug + @organization_name_from_slug ||= organization_slug_from_api_url&.titleize + end + + def project_slug_from_api_url + extract_slug(:project) + end + + def organization_slug_from_api_url + extract_slug(:organization) + end + + def extract_slug(capture) + return if api_url.blank? + + begin + url = Addressable::URI.parse(api_url) + rescue Addressable::URI::InvalidURIError + return nil + end + + @slug_match ||= url.path.match(%r{^/api/0/projects/+(?[^/]+)/+(?[^/|$]+)}) || {} + @slug_match[capture] + end + def validate_api_url_path - unless URI(api_url).path.starts_with?('/api/0/projects') - errors.add(:api_url, 'path needs to start with /api/0/projects') + return if api_url.blank? + + begin + unless Addressable::URI.parse(api_url).path.starts_with?('/api/0/projects') + errors.add(:api_url, 'path needs to start with /api/0/projects') + end + rescue Addressable::URI::InvalidURIError end - rescue URI::InvalidURIError end end end diff --git a/db/migrate/20190115092821_add_columns_project_error_tracking_settings.rb b/db/migrate/20190115092821_add_columns_project_error_tracking_settings.rb new file mode 100644 index 00000000000..190b6f958fd --- /dev/null +++ b/db/migrate/20190115092821_add_columns_project_error_tracking_settings.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class AddColumnsProjectErrorTrackingSettings < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_column :project_error_tracking_settings, :project_name, :string + add_column :project_error_tracking_settings, :organization_name, :string + + change_column_default :project_error_tracking_settings, :enabled, from: true, to: false + + change_column_null :project_error_tracking_settings, :api_url, true + end +end diff --git a/db/schema.rb b/db/schema.rb index 4cc870ba6c8..4b6e4992056 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1574,10 +1574,12 @@ ActiveRecord::Schema.define(version: 20190131122559) do end create_table "project_error_tracking_settings", primary_key: "project_id", id: :integer, force: :cascade do |t| - t.boolean "enabled", default: true, null: false - t.string "api_url", null: false + t.boolean "enabled", default: false, null: false + t.string "api_url" t.string "encrypted_token" t.string "encrypted_token_iv" + t.string "project_name" + t.string "organization_name" end create_table "project_features", force: :cascade do |t| diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index 7987533978c..add7ee58da6 100644 --- a/lib/gitlab/import_export/import_export.yml +++ b/lib/gitlab/import_export/import_export.yml @@ -166,6 +166,7 @@ excluded_attributes: error_tracking_setting: - :encrypted_token - :encrypted_token_iv + - :enabled methods: labels: diff --git a/spec/factories/project_error_tracking_settings.rb b/spec/factories/project_error_tracking_settings.rb index fbd8dfd395c..be30bd0260a 100644 --- a/spec/factories/project_error_tracking_settings.rb +++ b/spec/factories/project_error_tracking_settings.rb @@ -6,5 +6,7 @@ FactoryBot.define do api_url 'https://gitlab.com/api/0/projects/sentry-org/sentry-project' enabled true token 'access_token_123' + project_name 'Sentry Project' + organization_name 'Sentry Org' end end diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index fe2087e8fc3..baca8f6d542 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -603,5 +603,6 @@ ResourceLabelEvent: ErrorTracking::ProjectErrorTrackingSetting: - id - api_url -- enabled - project_id +- project_name +- organization_name diff --git a/spec/models/error_tracking/project_error_tracking_setting_spec.rb b/spec/models/error_tracking/project_error_tracking_setting_spec.rb index 2f8ab21d4b2..d30228b863c 100644 --- a/spec/models/error_tracking/project_error_tracking_setting_spec.rb +++ b/spec/models/error_tracking/project_error_tracking_setting_spec.rb @@ -15,9 +15,11 @@ describe ErrorTracking::ProjectErrorTrackingSetting do describe 'Validations' do context 'when api_url is over 255 chars' do - it 'fails validation' do + before do subject.api_url = 'https://' + 'a' * 250 + end + it 'fails validation' do expect(subject).not_to be_valid expect(subject.errors.messages[:api_url]).to include('is too long (maximum is 255 characters)') end @@ -31,6 +33,34 @@ describe ErrorTracking::ProjectErrorTrackingSetting do end end + context 'presence validations' do + using RSpec::Parameterized::TableSyntax + + valid_api_url = 'http://example.com/api/0/projects/org-slug/proj-slug/' + valid_token = 'token' + + where(:enabled, :token, :api_url, :valid?) do + true | nil | nil | false + true | nil | valid_api_url | false + true | valid_token | nil | false + true | valid_token | valid_api_url | true + false | nil | nil | true + false | nil | valid_api_url | true + false | valid_token | nil | true + false | valid_token | valid_api_url | true + end + + with_them do + before do + subject.enabled = enabled + subject.token = token + subject.api_url = api_url + end + + it { expect(subject.valid?).to eq(valid?) } + end + end + context 'URL path' do it 'fails validation with wrong path' do subject.api_url = 'http://gitlab.com/project1/something' @@ -45,6 +75,16 @@ describe ErrorTracking::ProjectErrorTrackingSetting do expect(subject).to be_valid end end + + context 'non ascii chars in api_url' do + before do + subject.api_url = 'http://gitlab.com/api/0/projects/project1/something€' + end + + it 'fails validation' do + expect(subject).not_to be_valid + end + end end describe '#sentry_external_url' do @@ -106,4 +146,138 @@ describe ErrorTracking::ProjectErrorTrackingSetting do end end end + + describe '#list_sentry_projects' do + let(:projects) { [:list, :of, :projects] } + let(:sentry_client) { spy(:sentry_client) } + + it 'calls sentry client' do + expect(subject).to receive(:sentry_client).and_return(sentry_client) + expect(sentry_client).to receive(:list_projects).and_return(projects) + + result = subject.list_sentry_projects + + expect(result).to eq(projects: projects) + end + end + + context 'slugs' do + shared_examples_for 'slug from api_url' do |method, slug| + context 'when api_url is correct' do + before do + subject.api_url = 'http://gitlab.com/api/0/projects/org-slug/project-slug/' + end + + it 'returns slug' do + expect(subject.public_send(method)).to eq(slug) + end + end + + context 'when api_url is blank' do + before do + subject.api_url = nil + end + + it 'returns nil' do + expect(subject.public_send(method)).to be_nil + end + end + end + + it_behaves_like 'slug from api_url', :project_slug, 'project-slug' + it_behaves_like 'slug from api_url', :organization_slug, 'org-slug' + end + + context 'names from api_url' do + shared_examples_for 'name from api_url' do |name, titleized_slug| + context 'name is present in DB' do + it 'returns name from DB' do + subject[name] = 'Sentry name' + subject.api_url = 'http://gitlab.com/api/0/projects/org-slug/project-slug' + + expect(subject.public_send(name)).to eq('Sentry name') + end + end + + context 'name is null in DB' do + it 'titleizes and returns slug from api_url' do + subject[name] = nil + subject.api_url = 'http://gitlab.com/api/0/projects/org-slug/project-slug' + + expect(subject.public_send(name)).to eq(titleized_slug) + end + + it 'returns nil when api_url is incorrect' do + subject[name] = nil + subject.api_url = 'http://gitlab.com/api/0/projects/' + + expect(subject.public_send(name)).to be_nil + end + + it 'returns nil when api_url is blank' do + subject[name] = nil + subject.api_url = nil + + expect(subject.public_send(name)).to be_nil + end + end + end + + it_behaves_like 'name from api_url', :organization_name, 'Org Slug' + it_behaves_like 'name from api_url', :project_name, 'Project Slug' + end + + describe '.build_api_url_from' do + it 'correctly builds api_url with slugs' do + api_url = described_class.build_api_url_from( + api_host: 'http://sentry.com/', + organization_slug: 'org-slug', + project_slug: 'proj-slug' + ) + + expect(api_url).to eq('http://sentry.com/api/0/projects/org-slug/proj-slug/') + end + + it 'correctly builds api_url without slugs' do + api_url = described_class.build_api_url_from( + api_host: 'http://sentry.com/', + organization_slug: nil, + project_slug: nil + ) + + expect(api_url).to eq('http://sentry.com/api/0/projects/') + end + + it 'does not raise exception with invalid url' do + api_url = described_class.build_api_url_from( + api_host: ':::', + organization_slug: 'org-slug', + project_slug: 'proj-slug' + ) + + expect(api_url).to eq(':::') + end + end + + describe '#api_host' do + context 'when api_url exists' do + before do + subject.api_url = 'https://example.com/api/0/projects/org-slug/proj-slug/' + end + + it 'extracts the api_host from api_url' do + expect(subject.api_host).to eq('https://example.com/') + end + end + + context 'when api_url is nil' do + before do + subject.api_url = nil + end + + it 'returns nil' do + expect(subject.api_url).to eq(nil) + end + end + end end -- cgit v1.2.1