Merge branch '55199-schema-and-model-changes' into 'master'

DB and model changes for Sentry project selection dropdown

See merge request gitlab-org/gitlab-ce!24677

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/+(?<organization>[^/]+)/+(?<project>[^/|$]+)}) || {}
+      @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

db/migrate/20190115092821_add_columns_project_error_tracking_settings.rb

+# 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

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|

lib/gitlab/import_export/import_export.yml

@@ -166,6 +166,7 @@ excluded_attributes:
   error_tracking_setting:
     - :encrypted_token
     - :encrypted_token_iv
+    - :enabled
 
 methods:
   labels:

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

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

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