Use title and description fields for issue trackers

- instead of using properties
- backward compatibility has to be kept for now

app/models/project_services/bugzilla_service.rb

@@ -3,22 +3,14 @@
 class BugzillaService < 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
+  prop_accessor :project_url, :issues_url, :new_issue_url
 
-  def title
-    if self.properties && self.properties['title'].present?
-      self.properties['title']
-    else
+  def default_title
     'Bugzilla'
   end
-  end
 
-  def description
-    if self.properties && self.properties['description'].present?
-      self.properties['description']
-    else
-      'Bugzilla issue tracker'
-    end
+  def default_description
+    s_('IssueTracker|Bugzilla issue tracker')
   end
 
   def self.to_param

app/models/project_services/custom_issue_tracker_service.rb

@@ -5,24 +5,12 @@ class CustomIssueTrackerService < IssueTrackerService
 
   prop_accessor :title, :description, :project_url, :issues_url, :new_issue_url
 
-  def title
-    if self.properties && self.properties['title'].present?
-      self.properties['title']
-    else
+  def default_title
     'Custom Issue Tracker'
   end
-  end
-
-  def title=(value)
-    self.properties['title'] = value if self.properties
-  end
 
-  def description
-    if self.properties && self.properties['description'].present?
-      self.properties['description']
-    else
-      'Custom issue tracker'
-    end
+  def default_description
+    s_('IssueTracker|Custom issue tracker')
   end
 
   def self.to_param

app/models/project_services/gitlab_issue_tracker_service.rb

@@ -5,10 +5,18 @@ class GitlabIssueTrackerService < 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
+  prop_accessor :project_url, :issues_url, :new_issue_url
 
   default_value_for :default, true
 
+  def default_title
+    'GitLab'
+  end
+
+  def default_description
+    s_('IssueTracker|GitLab issue tracker')
+  end
+
   def self.to_param
     'gitlab'
   end

app/models/project_services/issue_tracker_service.rb

@@ -5,6 +5,8 @@ class IssueTrackerService < Service
 
   default_value_for :category, 'issue_tracker'
 
+  before_save :handle_properties
+
   # Pattern used to extract links from comments
   # Override this method on services that uses different patterns
   # This pattern does not support cross-project references
@@ -18,6 +20,37 @@ class IssueTrackerService < Service
     end
   end
 
+  # this  will be removed as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/63084
+  def title
+    if title_attribute = read_attribute(:title)
+      title_attribute
+    elsif self.properties && self.properties['title'].present?
+      self.properties['title']
+    else
+      default_title
+    end
+  end
+
+  # this  will be removed as part of https://gitlab.com/gitlab-org/gitlab-ce/issues/63084
+  def description
+    if description_attribute = read_attribute(:description)
+      description_attribute
+    elsif self.properties && self.properties['description'].present?
+      self.properties['description']
+    else
+      default_description
+    end
+  end
+
+  def handle_properties
+    properties.slice('title', 'description').each do |key, _|
+      current_value = self.properties.delete(key)
+      value = attribute_changed?(key) ? attribute_change(key).last : current_value
+
+      write_attribute(key, value)
+    end
+  end
+
   def default?
     default
   end

app/models/project_services/jira_service.rb

@@ -17,7 +17,7 @@ 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, :title, :description
+  prop_accessor :username, :password, :url, :api_url, :jira_issue_transition_id
 
   before_update :reset_password
 
@@ -37,7 +37,6 @@ class JiraService < IssueTrackerService
   def initialize_properties
     super do
       self.properties = {
-        title: issues_tracker['title'],
         url: issues_tracker['url'],
         api_url: issues_tracker['api_url']
       }
@@ -74,21 +73,13 @@ class JiraService < IssueTrackerService
     [Jira service documentation](#{help_page_url('user/project/integrations/jira')})."
   end
 
-  def title
-    if self.properties && self.properties['title'].present?
-      self.properties['title']
-    else
+  def default_title
     'Jira'
   end
-  end
 
-  def description
-    if self.properties && self.properties['description'].present?
-      self.properties['description']
-    else
+  def default_description
     s_('JiraService|Jira issue tracker')
   end
-  end
 
   def self.to_param
     'jira'

app/models/project_services/redmine_service.rb

@@ -3,22 +3,14 @@
 class RedmineService < 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
+  prop_accessor :project_url, :issues_url, :new_issue_url
 
-  def title
-    if self.properties && self.properties['title'].present?
-      self.properties['title']
-    else
+  def default_title
     'Redmine'
   end
-  end
 
-  def description
-    if self.properties && self.properties['description'].present?
-      self.properties['description']
-    else
-      'Redmine issue tracker'
-    end
+  def default_description
+    s_('IssueTracker|Redmine issue tracker')
   end
 
   def self.to_param

app/models/project_services/youtrack_service.rb

@@ -3,7 +3,7 @@
 class YoutrackService < IssueTrackerService
   validates :project_url, :issues_url, presence: true, public_url: true, if: :activated?
 
-  prop_accessor :description, :project_url, :issues_url
+  prop_accessor :project_url, :issues_url
 
   # {PROJECT-KEY}-{NUMBER} Examples: YT-1, PRJ-1, gl-030
   def self.reference_pattern(only_long: false)
@@ -14,16 +14,12 @@ class YoutrackService < IssueTrackerService
     end
   end
 
-  def title
+  def default_title
     'YouTrack'
   end
 
-  def description
-    if self.properties && self.properties['description'].present?
-      self.properties['description']
-    else
-      'YouTrack issue tracker'
-    end
+  def default_description
+    s_('IssueTracker|YouTrack issue tracker')
   end
 
   def self.to_param

app/models/service.rb

@@ -129,7 +129,7 @@ class Service < ApplicationRecord
 
   def api_field_names
     fields.map { |field| field[:name] }
-      .reject { |field_name| field_name =~ /(password|token|key)/ }
+      .reject { |field_name| field_name =~ /(password|token|key|title|description)/ }
   end
 
   def global_fields

db/migrate/20190625115224_add_description_to_services.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddDescriptionToServices < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :services, :description, :string, limit: 500
+  end
+end

db/schema.rb

@@ -3028,6 +3028,7 @@ ActiveRecord::Schema.define(version: 20190627051902) do
     t.boolean "job_events", default: false, null: false
     t.boolean "confidential_note_events", default: true
     t.boolean "deployment_events", default: false, null: false
+    t.string "description", limit: 500
     t.index ["project_id"], name: "index_services_on_project_id", using: :btree
     t.index ["template"], name: "index_services_on_template", using: :btree
     t.index ["type"], name: "index_services_on_type", using: :btree

locale/gitlab.pot

@@ -5583,6 +5583,21 @@ msgstr ""
 msgid "IssueBoards|Boards"
 msgstr ""
 
+msgid "IssueTracker|Bugzilla issue tracker"
+msgstr ""
+
+msgid "IssueTracker|Custom issue tracker"
+msgstr ""
+
+msgid "IssueTracker|GitLab issue tracker"
+msgstr ""
+
+msgid "IssueTracker|Redmine issue tracker"
+msgstr ""
+
+msgid "IssueTracker|YouTrack issue tracker"
+msgstr ""
+
 msgid "Issues"
 msgstr ""
 

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

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

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

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

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

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
 
-      it 'is correct' do
-        expect(@service.title).to eq('Jira One')
-        expect(@service.description).to eq('Jira One issue tracker')
+    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'
+
+        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

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

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

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
 

spec/support/shared_examples/models/services_fields_shared_examples.rb

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