From ddbbf453c76144ac60c67e783424faf843c8efa1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= <jarka@gitlab.com>
Date: Wed, 26 Jun 2019 16:03:57 +0200
Subject: Use title and description fields for issue trackers

- instead of using properties
- backward compatibility has to be kept for now
---
 spec/factories/services.rb                         |  34 ++++-
 .../gitlab/import_export/safe_model_attributes.yml |   1 +
 .../project_services/bugzilla_service_spec.rb      |  45 +++++++
 .../custom_issue_tracker_service_spec.rb           |  43 +++++++
 .../gitlab_issue_tracker_service_spec.rb           |  43 +++++++
 spec/models/project_services/jira_service_spec.rb  | 138 ++++++++++++++++-----
 .../project_services/redmine_service_spec.rb       |  43 +++++++
 .../project_services/youtrack_service_spec.rb      |  43 +++++++
 spec/models/service_spec.rb                        |   9 +-
 .../models/services_fields_shared_examples.rb      |  31 +++++
 10 files changed, 394 insertions(+), 36 deletions(-)
 create mode 100644 spec/support/shared_examples/models/services_fields_shared_examples.rb

(limited to 'spec')

diff --git a/spec/factories/services.rb b/spec/factories/services.rb
index 763909f30bd..ecb481ed84a 100644
--- a/spec/factories/services.rb
+++ b/spec/factories/services.rb
@@ -6,8 +6,6 @@ FactoryBot.define do
 
   factory :custom_issue_tracker_service, class: CustomIssueTrackerService do
     project
-    type 'CustomIssueTrackerService'
-    category 'issue_tracker'
     active true
     properties(
       project_url: 'https://project.url.com',
@@ -54,6 +52,38 @@ FactoryBot.define do
     )
   end
 
+  factory :bugzilla_service do
+    project
+    active true
+    issue_tracker
+  end
+
+  factory :redmine_service do
+    project
+    active true
+    issue_tracker
+  end
+
+  factory :youtrack_service do
+    project
+    active true
+    issue_tracker
+  end
+
+  factory :gitlab_issue_tracker_service do
+    project
+    active true
+    issue_tracker
+  end
+
+  trait :issue_tracker do
+    properties(
+      project_url: 'http://issue-tracker.example.com',
+      issues_url: 'http://issue-tracker.example.com',
+      new_issue_url: 'http://issue-tracker.example.com'
+    )
+  end
+
   factory :jira_cloud_service, class: JiraService do
     project
     active true
diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml
index a406c25b1d8..2758023fb17 100644
--- a/spec/lib/gitlab/import_export/safe_model_attributes.yml
+++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml
@@ -429,6 +429,7 @@ Service:
 - confidential_issues_events
 - confidential_note_events
 - deployment_events
+- description
 ProjectHook:
 - id
 - url
diff --git a/spec/models/project_services/bugzilla_service_spec.rb b/spec/models/project_services/bugzilla_service_spec.rb
index 6818db48fee..d5b0f94f461 100644
--- a/spec/models/project_services/bugzilla_service_spec.rb
+++ b/spec/models/project_services/bugzilla_service_spec.rb
@@ -32,4 +32,49 @@ describe BugzillaService do
       it { is_expected.not_to validate_presence_of(:new_issue_url) }
     end
   end
+
+  context 'overriding properties' do
+    let(:default_title) { 'JIRA' }
+    let(:default_description) { 'JiraService|Jira issue tracker' }
+    let(:url) { 'http://bugzilla.example.com' }
+    let(:access_params) do
+      { project_url: url, issues_url: url, new_issue_url: url }
+    end
+
+    # this  will be removed as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/63084
+    context 'when data are stored in properties' do
+      let(:properties) { access_params.merge(title: title, description: description) }
+      let(:service) { create(:bugzilla_service, properties: properties) }
+
+      include_examples 'issue tracker fields'
+    end
+
+    context 'when data are stored in separated fields' do
+      let(:service) do
+        create(:bugzilla_service, title: title, description: description, properties: access_params)
+      end
+
+      include_examples 'issue tracker fields'
+    end
+
+    context 'when data are stored in both properties and separated fields' do
+      let(:properties) { access_params.merge(title: 'wrong title', description: 'wrong description') }
+      let(:service) do
+        create(:bugzilla_service, title: title, description: description, properties: properties)
+      end
+
+      include_examples 'issue tracker fields'
+    end
+
+    context 'when no title & description are set' do
+      let(:service) do
+        create(:bugzilla_service, properties: access_params)
+      end
+
+      it 'returns default values' do
+        expect(service.title).to eq('Bugzilla')
+        expect(service.description).to eq('Bugzilla issue tracker')
+      end
+    end
+  end
 end
diff --git a/spec/models/project_services/custom_issue_tracker_service_spec.rb b/spec/models/project_services/custom_issue_tracker_service_spec.rb
index f0e7551693d..56b0bda6626 100644
--- a/spec/models/project_services/custom_issue_tracker_service_spec.rb
+++ b/spec/models/project_services/custom_issue_tracker_service_spec.rb
@@ -48,4 +48,47 @@ describe CustomIssueTrackerService do
       end
     end
   end
+
+  context 'overriding properties' do
+    let(:url) { 'http://custom.example.com' }
+    let(:access_params) do
+      { project_url: url, issues_url: url, new_issue_url: url }
+    end
+
+    # this  will be removed as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/63084
+    context 'when data are stored in properties' do
+      let(:properties) { access_params.merge(title: title, description: description) }
+      let(:service) { create(:custom_issue_tracker_service, properties: properties) }
+
+      include_examples 'issue tracker fields'
+    end
+
+    context 'when data are stored in separated fields' do
+      let(:service) do
+        create(:custom_issue_tracker_service, title: title, description: description, properties: access_params)
+      end
+
+      include_examples 'issue tracker fields'
+    end
+
+    context 'when data are stored in both properties and separated fields' do
+      let(:properties) { access_params.merge(title: 'wrong title', description: 'wrong description') }
+      let(:service) do
+        create(:custom_issue_tracker_service, title: title, description: description, properties: properties)
+      end
+
+      include_examples 'issue tracker fields'
+    end
+
+    context 'when no title & description are set' do
+      let(:service) do
+        create(:custom_issue_tracker_service, properties: access_params)
+      end
+
+      it 'returns default values' do
+        expect(service.title).to eq('Custom Issue Tracker')
+        expect(service.description).to eq('Custom issue tracker')
+      end
+    end
+  end
 end
diff --git a/spec/models/project_services/gitlab_issue_tracker_service_spec.rb b/spec/models/project_services/gitlab_issue_tracker_service_spec.rb
index 11f96c03d46..a3726f09dc5 100644
--- a/spec/models/project_services/gitlab_issue_tracker_service_spec.rb
+++ b/spec/models/project_services/gitlab_issue_tracker_service_spec.rb
@@ -51,4 +51,47 @@ describe GitlabIssueTrackerService do
       end
     end
   end
+
+  context 'overriding properties' do
+    let(:url) { 'http://gitlab.example.com' }
+    let(:access_params) do
+      { project_url: url, issues_url: url, new_issue_url: url }
+    end
+
+    # this  will be removed as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/63084
+    context 'when data are stored in properties' do
+      let(:properties) { access_params.merge(title: title, description: description) }
+      let(:service) { create(:gitlab_issue_tracker_service, properties: properties) }
+
+      include_examples 'issue tracker fields'
+    end
+
+    context 'when data are stored in separated fields' do
+      let(:service) do
+        create(:gitlab_issue_tracker_service, title: title, description: description, properties: access_params)
+      end
+
+      include_examples 'issue tracker fields'
+    end
+
+    context 'when data are stored in both properties and separated fields' do
+      let(:properties) { access_params.merge(title: 'wrong title', description: 'wrong description') }
+      let(:service) do
+        create(:gitlab_issue_tracker_service, title: title, description: description, properties: properties)
+      end
+
+      include_examples 'issue tracker fields'
+    end
+
+    context 'when no title & description are set' do
+      let(:service) do
+        create(:gitlab_issue_tracker_service, properties: access_params)
+      end
+
+      it 'returns default values' do
+        expect(service.title).to eq('GitLab')
+        expect(service.description).to eq('GitLab issue tracker')
+      end
+    end
+  end
 end
diff --git a/spec/models/project_services/jira_service_spec.rb b/spec/models/project_services/jira_service_spec.rb
index fc08457a3c5..9b122d85293 100644
--- a/spec/models/project_services/jira_service_spec.rb
+++ b/spec/models/project_services/jira_service_spec.rb
@@ -115,6 +115,70 @@ describe JiraService do
     end
   end
 
+  describe '#create' do
+    let(:params) do
+      {
+        project: create(:project), title: 'custom title', description: 'custom description'
+      }
+    end
+
+    subject { described_class.create(params) }
+
+    it 'does not store title & description into properties' do
+      expect(subject.properties.keys).not_to include('title', 'description')
+    end
+
+    it 'sets title & description correctly' do
+      service = subject
+
+      expect(service.title).to eq('custom title')
+      expect(service.description).to eq('custom description')
+    end
+  end
+
+  context 'overriding properties' do
+    let(:url) { 'http://issue_tracker.example.com' }
+    let(:access_params) do
+      { url: url, username: 'username', password: 'password' }
+    end
+
+    # this  will be removed as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/63084
+    context 'when data are stored in properties' do
+      let(:properties) { access_params.merge(title: title, description: description) }
+      let(:service) { create(:jira_service, properties: properties) }
+
+      include_examples 'issue tracker fields'
+    end
+
+    context 'when data are stored in separated fields' do
+      let(:service) do
+        create(:jira_service, title: title, description: description, properties: access_params)
+      end
+
+      include_examples 'issue tracker fields'
+    end
+
+    context 'when data are stored in both properties and separated fields' do
+      let(:properties) { access_params.merge(title: 'wrong title', description: 'wrong description') }
+      let(:service) do
+        create(:jira_service, title: title, description: description, properties: properties)
+      end
+
+      include_examples 'issue tracker fields'
+    end
+
+    context 'when no title & description are set' do
+      let(:service) do
+        create(:jira_service, properties: access_params)
+      end
+
+      it 'returns default values' do
+        expect(service.title).to eq('Jira')
+        expect(service.description).to eq('Jira issue tracker')
+      end
+    end
+  end
+
   describe '#close_issue' do
     let(:custom_base_url) { 'http://custom_url' }
     let(:user)    { create(:user) }
@@ -450,36 +514,54 @@ describe JiraService do
   end
 
   describe 'description and title' do
-    let(:project) { create(:project) }
+    let(:title) { 'Jira One' }
+    let(:description) { 'Jira One issue tracker' }
+    let(:properties) do
+      {
+        url: 'http://jira.example.com/web',
+        username: 'mic',
+        password: 'password',
+        title: title,
+        description: description
+      }
+    end
 
     context 'when it is not set' do
-      before do
-        @service = project.create_jira_service(active: true)
-      end
+      it 'default values are returned' do
+        service = create(:jira_service)
 
-      after do
-        @service.destroy!
+        expect(service.title).to eq('Jira')
+        expect(service.description).to eq('Jira issue tracker')
       end
+    end
 
-      it 'is initialized' do
-        expect(@service.title).to eq('Jira')
-        expect(@service.description).to eq('Jira issue tracker')
+    context 'when it is set in properties' do
+      it 'values from properties are returned' do
+        service = create(:jira_service, properties: properties)
+
+        expect(service.title).to eq(title)
+        expect(service.description).to eq(description)
       end
     end
 
-    context 'when it is set' do
-      before do
-        properties = { 'title' => 'Jira One', 'description' => 'Jira One issue tracker' }
-        @service = project.create_jira_service(active: true, properties: properties)
-      end
+    context 'when it is in title & description fields' do
+      it 'values from title and description fields are returned' do
+        service = create(:jira_service, title: title, description: description)
 
-      after do
-        @service.destroy!
+        expect(service.title).to eq(title)
+        expect(service.description).to eq(description)
       end
+    end
+
+    context 'when it is in both properites & title & description fields' do
+      it 'values from title and description fields are returned' do
+        title2 = 'Jira 2'
+        description2 = 'Jira description 2'
 
-      it 'is correct' do
-        expect(@service.title).to eq('Jira One')
-        expect(@service.description).to eq('Jira One issue tracker')
+        service = create(:jira_service, title: title2, description: description2, properties: properties)
+
+        expect(service.title).to eq(title2)
+        expect(service.description).to eq(description2)
       end
     end
   end
@@ -505,29 +587,21 @@ describe JiraService do
   end
 
   describe 'project and issue urls' do
-    let(:project) { create(:project) }
-
     context 'when gitlab.yml was initialized' do
-      before do
+      it 'is prepopulated with the settings' do
         settings = {
           'jira' => {
-            'title' => 'Jira',
             'url' => 'http://jira.sample/projects/project_a',
             'api_url' => 'http://jira.sample/api'
           }
         }
         allow(Gitlab.config).to receive(:issues_tracker).and_return(settings)
-        @service = project.create_jira_service(active: true)
-      end
 
-      after do
-        @service.destroy!
-      end
+        project = create(:project)
+        service = project.create_jira_service(active: true)
 
-      it 'is prepopulated with the settings' do
-        expect(@service.properties['title']).to eq('Jira')
-        expect(@service.properties['url']).to eq('http://jira.sample/projects/project_a')
-        expect(@service.properties['api_url']).to eq('http://jira.sample/api')
+        expect(service.properties['url']).to eq('http://jira.sample/projects/project_a')
+        expect(service.properties['api_url']).to eq('http://jira.sample/api')
       end
     end
   end
diff --git a/spec/models/project_services/redmine_service_spec.rb b/spec/models/project_services/redmine_service_spec.rb
index ac570ac27e1..806e3695962 100644
--- a/spec/models/project_services/redmine_service_spec.rb
+++ b/spec/models/project_services/redmine_service_spec.rb
@@ -40,4 +40,47 @@ describe RedmineService do
       expect(described_class.reference_pattern.match('#123')[:issue]).to eq('123')
     end
   end
+
+  context 'overriding properties' do
+    let(:url) { 'http://redmine.example.com' }
+    let(:access_params) do
+      { project_url: url, issues_url: url, new_issue_url: url }
+    end
+
+    # this  will be removed as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/63084
+    context 'when data are stored in properties' do
+      let(:properties) { access_params.merge(title: title, description: description) }
+      let(:service) { create(:redmine_service, properties: properties) }
+
+      include_examples 'issue tracker fields'
+    end
+
+    context 'when data are stored in separated fields' do
+      let(:service) do
+        create(:redmine_service, title: title, description: description, properties: access_params)
+      end
+
+      include_examples 'issue tracker fields'
+    end
+
+    context 'when data are stored in both properties and separated fields' do
+      let(:properties) { access_params.merge(title: 'wrong title', description: 'wrong description') }
+      let(:service) do
+        create(:redmine_service, title: title, description: description, properties: properties)
+      end
+
+      include_examples 'issue tracker fields'
+    end
+
+    context 'when no title & description are set' do
+      let(:service) do
+        create(:redmine_service, properties: access_params)
+      end
+
+      it 'returns default values' do
+        expect(service.title).to eq('Redmine')
+        expect(service.description).to eq('Redmine issue tracker')
+      end
+    end
+  end
 end
diff --git a/spec/models/project_services/youtrack_service_spec.rb b/spec/models/project_services/youtrack_service_spec.rb
index bf9d892f66c..b47ef6702b4 100644
--- a/spec/models/project_services/youtrack_service_spec.rb
+++ b/spec/models/project_services/youtrack_service_spec.rb
@@ -37,4 +37,47 @@ describe YoutrackService do
       expect(described_class.reference_pattern.match('YT-123')[:issue]).to eq('YT-123')
     end
   end
+
+  context 'overriding properties' do
+    let(:url) { 'http://youtrack.example.com' }
+    let(:access_params) do
+      { project_url: url, issues_url: url, new_issue_url: url }
+    end
+
+    # this  will be removed as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/63084
+    context 'when data are stored in properties' do
+      let(:properties) { access_params.merge(title: title, description: description) }
+      let(:service) { create(:youtrack_service, properties: properties) }
+
+      include_examples 'issue tracker fields'
+    end
+
+    context 'when data are stored in separated fields' do
+      let(:service) do
+        create(:youtrack_service, title: title, description: description, properties: access_params)
+      end
+
+      include_examples 'issue tracker fields'
+    end
+
+    context 'when data are stored in both properties and separated fields' do
+      let(:properties) { access_params.merge(title: 'wrong title', description: 'wrong description') }
+      let(:service) do
+        create(:youtrack_service, title: title, description: description, properties: properties)
+      end
+
+      include_examples 'issue tracker fields'
+    end
+
+    context 'when no title & description are set' do
+      let(:service) do
+        create(:youtrack_service, properties: access_params)
+      end
+
+      it 'returns default values' do
+        expect(service.title).to eq('YouTrack')
+        expect(service.description).to eq('YouTrack issue tracker')
+      end
+    end
+  end
 end
diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb
index d442c73c118..0797b9a9d83 100644
--- a/spec/models/service_spec.rb
+++ b/spec/models/service_spec.rb
@@ -244,7 +244,8 @@ describe Service do
     let(:service) do
       GitlabIssueTrackerService.create(
         project: create(:project),
-        title: 'random title'
+        title: 'random title',
+        project_url: 'http://gitlab.example.com'
       )
     end
 
@@ -252,8 +253,12 @@ describe Service do
       expect { service }.not_to raise_error
     end
 
+    it 'sets title correctly' do
+      expect(service.title).to eq('random title')
+    end
+
     it 'creates the properties' do
-      expect(service.properties).to eq({ "title" => "random title" })
+      expect(service.properties).to eq({ "project_url" => "http://gitlab.example.com" })
     end
   end
 
diff --git a/spec/support/shared_examples/models/services_fields_shared_examples.rb b/spec/support/shared_examples/models/services_fields_shared_examples.rb
new file mode 100644
index 00000000000..6fbd0da9383
--- /dev/null
+++ b/spec/support/shared_examples/models/services_fields_shared_examples.rb
@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+shared_examples 'issue tracker fields' do
+  let(:title) { 'custom title' }
+  let(:description) { 'custom description' }
+  let(:url) { 'http://issue_tracker.example.com' }
+
+  context 'when data are stored in the properties' do
+    describe '#update' do
+      before do
+        service.update(title: 'new_title', description: 'new description')
+      end
+
+      it 'removes title and description from properties' do
+        expect(service.reload.properties).not_to include('title', 'description')
+      end
+
+      it 'stores title & description in services table' do
+        expect(service.read_attribute(:title)).to eq('new_title')
+        expect(service.read_attribute(:description)).to eq('new description')
+      end
+    end
+
+    describe 'reading fields' do
+      it 'returns correct values' do
+        expect(service.title).to eq(title)
+        expect(service.description).to eq(description)
+      end
+    end
+  end
+end
-- 
cgit v1.2.1