Use data_fields for issue trackers63082-jira-service-data

- create and update data only in data fields tables
- keep backwards compatibility for reading
from properties
- simlify factories

22 files changed, 394 insertions, 305 deletions

diff --git a/app/models/project_services/bugzilla_service.rb b/app/models/project_services/bugzilla_service.rb
index 8b79b5e9f0c..3d7fa973c7e 100644
--- a/app/models/project_services/bugzilla_service.rb
+++ b/app/models/project_services/bugzilla_service.rb
@@ -1,10 +1,6 @@
 # frozen_string_literal: true
 
 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
diff --git a/app/models/project_services/data_fields.rb b/app/models/project_services/data_fields.rb
index 438d85098c8..ee11847fa02 100644
--- a/app/models/project_services/data_fields.rb
+++ b/app/models/project_services/data_fields.rb
@@ -3,8 +3,66 @@
 module DataFields
   extend ActiveSupport::Concern
 
+  class_methods do
+    # Provide convenient accessor methods
+    # for each serialized property.
+    # Also keep track of updated properties in a similar way as ActiveModel::Dirty
+    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}=(value)
+          #   data_fields.send('#{arg}=', value)
+          #   updated_properties['#{arg}'] = value unless updated_properties['#{arg}']
+          # end
+
+          def #{arg}_changed?
+            #{arg}_touched? && @old_data_fields['#{arg}'] != #{arg}
+          end
+
+          # def #{arg}_touched?
+          #   updated_properties.include?('#{arg}')
+          # end
+
+          def #{arg}_is
+            data_fields.#{arg}
+          end
+
+          def #{arg}_was
+            data_fields.#{arg}_was || updated_properties['#{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 updated_data_fields
+      @updated_data_fields ||= ActiveSupport::HashWithIndifferentAccess.new
+    end
   end
 end
diff --git a/app/models/project_services/gitlab_issue_tracker_service.rb b/app/models/project_services/gitlab_issue_tracker_service.rb
index 51032932eab..4011c308484 100644
--- a/app/models/project_services/gitlab_issue_tracker_service.rb
+++ b/app/models/project_services/gitlab_issue_tracker_service.rb
@@ -3,10 +3,6 @@
 class GitlabIssueTrackerService < IssueTrackerService
   include Gitlab::Routing
 
-  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
diff --git a/app/models/project_services/issue_tracker_service.rb b/app/models/project_services/issue_tracker_service.rb
index 3a1130ffc15..06efdc9014f 100644
--- a/app/models/project_services/issue_tracker_service.rb
+++ b/app/models/project_services/issue_tracker_service.rb
@@ -3,9 +3,12 @@
 class IssueTrackerService < Service
   validate :one_issue_tracker, if: :activated?, on: :manual_change
 
+  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 +46,30 @@ class IssueTrackerService < Service
   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
+
+    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
+
+    updated_properties.each do |key, value|
+      updated_properties[key] = data_values[key]
+    end
+    data_values.reject! { |key| data_fields.changed.include?(key) }
+
+    data_fields.assign_attributes(data_values) if data_values.present?
+
+    self.properties = {}
+  end
+
+  def data_fields(values = {})
+    issue_tracker_data || self.build_issue_tracker_data(values)
   end
 
   def default?
@@ -56,7 +77,7 @@ class IssueTrackerService < Service
   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 +101,21 @@ class IssueTrackerService < Service
     ]
   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']
+
+    return if project_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
diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb
index 0728c83005e..a3941340703 100644
--- a/app/models/project_services/jira_service.rb
+++ b/app/models/project_services/jira_service.rb
@@ -5,19 +5,10 @@ class JiraService < IssueTrackerService
   include ApplicationHelper
   include ActionView::Helpers::AssetUrlHelper
 
-  validates :url, public_url: true, presence: true, if: :activated?
-  validates :api_url, public_url: 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
-
   # 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
+  data_field :username, :password, :url, :api_url, :jira_issue_transition_id
 
   before_update :reset_password
 
@@ -35,24 +26,34 @@ class JiraService < IssueTrackerService
   end
 
   def initialize_properties
-    super do
-      self.properties = {
-        url: issues_tracker['url'],
-        api_url: issues_tracker['api_url']
-      }
-    end
+    {}
+  end
+
+  def data_fields(values = {})
+    jira_tracker_data || self.build_jira_tracker_data(values)
   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,
diff --git a/app/models/project_services/redmine_service.rb b/app/models/project_services/redmine_service.rb
index 5ca057ca833..ef66d88629e 100644
--- a/app/models/project_services/redmine_service.rb
+++ b/app/models/project_services/redmine_service.rb
@@ -1,10 +1,6 @@
 # frozen_string_literal: true
 
 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
diff --git a/app/models/project_services/youtrack_service.rb b/app/models/project_services/youtrack_service.rb
index f9de1f7dc49..37eb1c4d4c1 100644
--- a/app/models/project_services/youtrack_service.rb
+++ b/app/models/project_services/youtrack_service.rb
@@ -1,10 +1,6 @@
 # frozen_string_literal: true
 
 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
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 312c8d5b548..cc45679dd0e 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -1044,7 +1044,11 @@ module API
       expose :job_events
       # Expose serialized properties
       expose :properties do |service, options|
-        service.properties.slice(*service.api_field_names)
+        if service.properties.present?
+          service.properties.slice(*service.api_field_names)
+        else
+          service.data_fields.as_json.slice(*service.api_field_names)
+        end
       end
     end
 
diff --git a/lib/gitlab/usage_data.rb b/lib/gitlab/usage_data.rb
index a93301cb4ce..e6df1884a33 100644
--- a/lib/gitlab/usage_data.rb
+++ b/lib/gitlab/usage_data.rb
@@ -172,16 +172,16 @@ module Gitlab
       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)
-        )
+        services = Service.unscoped.where(type: :JiraService, active: true).includes(:jira_tracker_data)
+
+        counts = services.group_by do |service|
+          service.jira_tracker_data.url.include?('.atlassian.net') ? :cloud : :server
+        end
 
         {
-          projects_jira_server_active: services['server'] || 0,
-          projects_jira_cloud_active: services['cloud'] || 0,
-          projects_jira_active: services['server'] == -1 ? -1 : services.values.sum
+          projects_jira_server_active: counts[:server]&.count || 0,
+          projects_jira_cloud_active: counts[:cloud]&.count || 0,
+          projects_jira_active: services ? count(services) : -1
         }
       end
 
diff --git a/spec/controllers/projects/services_controller_spec.rb b/spec/controllers/projects/services_controller_spec.rb
index 0c074714bf3..4a678e63165 100644
--- a/spec/controllers/projects/services_controller_spec.rb
+++ b/spec/controllers/projects/services_controller_spec.rb
@@ -5,7 +5,9 @@ require 'spec_helper'
 describe Projects::ServicesController do
   let(:project) { create(:project, :repository) }
   let(:user)    { create(:user) }
-  let(:service) { create(:jira_service, project: project) }
+  let(:service) do
+    create(:jira_service, project: project).tap { |service| service.jira_tracker_data.destroy }
+  end
   let(:service_params) { { username: 'username', password: 'password', url: 'http://example.com' } }
 
   before do
diff --git a/spec/factories/services.rb b/spec/factories/services.rb
index b2d6ada91fa..ada042726aa 100644
--- a/spec/factories/services.rb
+++ b/spec/factories/services.rb
@@ -47,12 +47,23 @@ FactoryBot.define do
   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 'https://jira-api.example.com'
+      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 +91,31 @@ FactoryBot.define do
   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'
-    )
+    after(:build) do |service|
+      create(:jira_tracker_data,
+             service: service,
+             url: 'https://mysite.atlassian.net',
+             username: 'jira_user',
+             password: 'my-secret-password'
+            )
+    end
   end
 
   factory :hipchat_service do
@@ -102,15 +124,21 @@ FactoryBot.define do
     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
-    after(:build) do |service|
-      allow(service).to receive(:handle_properties)
+    jira_tracker_data nil
+    issue_tracker_data nil
+
+    after(:build) do
+      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
diff --git a/spec/features/issuables/markdown_references/jira_spec.rb b/spec/features/issuables/markdown_references/jira_spec.rb
index 8085918f533..b64c63c1c81 100644
--- a/spec/features/issuables/markdown_references/jira_spec.rb
+++ b/spec/features/issuables/markdown_references/jira_spec.rb
@@ -15,8 +15,8 @@ describe "Jira", :js do
     before do
       remotelink = double(:remotelink, all: [], build: double(save!: true))
 
-      stub_request(:get, "https://jira.example.com/rest/api/2/issue/JIRA-5")
-      stub_request(:post, "https://jira.example.com/rest/api/2/issue/JIRA-5/comment")
+      stub_request(:get, "https://jira-api.example.com/rest/api/2/issue/JIRA-5")
+      stub_request(:post, "https://jira-api.example.com/rest/api/2/issue/JIRA-5/comment")
       allow_any_instance_of(JIRA::Resource::Issue).to receive(:remotelink).and_return(remotelink)
 
       sign_in(user)
diff --git a/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb b/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb
index 7eb63fea413..47ea273ef3a 100644
--- a/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb
+++ b/spec/lib/banzai/pipeline/gfm_pipeline_spec.rb
@@ -34,7 +34,7 @@ describe Banzai::Pipeline::GfmPipeline do
         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 @@ describe Banzai::Pipeline::GfmPipeline do
         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 @@ describe Banzai::Pipeline::GfmPipeline do
         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
diff --git a/spec/models/project_services/bugzilla_service_spec.rb b/spec/models/project_services/bugzilla_service_spec.rb
index 74c85a13c88..4ad8f2c1f17 100644
--- a/spec/models/project_services/bugzilla_service_spec.rb
+++ b/spec/models/project_services/bugzilla_service_spec.rb
@@ -8,31 +8,6 @@ describe BugzillaService do
     it { is_expected.to have_one :service_hook }
   end
 
-  describe 'Validations' do
-    context 'when service is active' do
-      before do
-        subject.active = true
-      end
-
-      it { is_expected.to validate_presence_of(:project_url) }
-      it { is_expected.to validate_presence_of(:issues_url) }
-      it { is_expected.to validate_presence_of(:new_issue_url) }
-      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
-    end
-
-    context 'when service is inactive' do
-      before do
-        subject.active = false
-      end
-
-      it { is_expected.not_to validate_presence_of(:project_url) }
-      it { is_expected.not_to validate_presence_of(:issues_url) }
-      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' }
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 0c4fc290a13..a2c38063df6 100644
--- a/spec/models/project_services/gitlab_issue_tracker_service_spec.rb
+++ b/spec/models/project_services/gitlab_issue_tracker_service_spec.rb
@@ -8,21 +8,6 @@ describe GitlabIssueTrackerService do
     it { is_expected.to have_one :service_hook }
   end
 
-  describe 'Validations' do
-    context 'when service is active' do
-      subject { described_class.new(project: create(:project), active: true) }
-
-      it { is_expected.to validate_presence_of(:issues_url) }
-      it_behaves_like 'issue tracker service URL attribute', :issues_url
-    end
-
-    context 'when service is inactive' do
-      subject { described_class.new(project: create(:project), active: false) }
-
-      it { is_expected.not_to validate_presence_of(:issues_url) }
-    end
-  end
-
   describe 'project and issue urls' do
     let(:project) { create(:project) }
     let(:service) { project.create_gitlab_issue_tracker_service(active: true) }
diff --git a/spec/models/project_services/issue_tracker_service_spec.rb b/spec/models/project_services/issue_tracker_service_spec.rb
index 2fc4d69c2db..f1cdee5c4a3 100644
--- a/spec/models/project_services/issue_tracker_service_spec.rb
+++ b/spec/models/project_services/issue_tracker_service_spec.rb
@@ -7,7 +7,7 @@ describe IssueTrackerService do
     let(:project) { create :project }
 
     describe 'only one issue tracker per project' do
-      let(:service) { RedmineService.new(project: project, active: true) }
+      let(:service) { RedmineService.new(project: project, active: true, issue_tracker_data: build(:issue_tracker_data)) }
 
       before do
         create(:custom_issue_tracker_service, project: project)
diff --git a/spec/models/project_services/jira_service_spec.rb b/spec/models/project_services/jira_service_spec.rb
index ae109aadcc0..33112296a03 100644
--- a/spec/models/project_services/jira_service_spec.rb
+++ b/spec/models/project_services/jira_service_spec.rb
@@ -6,10 +6,18 @@ describe JiraService do
   include Gitlab::Routing
   include AssetsHelpers
 
+  let(:title) { 'custom title' }
+  let(:description) { 'custom description' }
+  let(:url) { 'http://jira.example.com' }
+  let(:api_url) { 'http://api-jira.example.com' }
+  let(:username) { 'jira-username' }
+  let(:password) { 'jira-password' }
+  let(:transition_id) { 'test27' }
+
   describe '#options' do
     let(:service) do
-      described_class.new(
-        project: build_stubbed(:project),
+      described_class.create(
+        project: create(:project),
         active: true,
         username: 'username',
         password: 'test',
@@ -46,55 +54,227 @@ describe JiraService do
   describe '#create' do
     let(:params) do
       {
-        project: create(:project), title: 'custom title', description: 'custom description'
+        project: create(:project),
+        title: 'custom title', description: 'custom description',
+        url: url, api_url: api_url,
+        username: username, password: password,
+        jira_issue_transition_id: transition_id
       }
     end
 
     subject { described_class.create(params) }
 
-    it 'does not store title & description into properties' do
-      expect(subject.properties.keys).not_to include('title', 'description')
+    it 'does not store data into properties' do
+      expect(subject.properties).to be_nil
     end
 
-    it 'sets title & description correctly' do
+    it 'sets title  correctly' do
+      service = subject
+
+      expect(service.title).to eq('custom title')
+    end
+
+    it 'sets service data correctly' do
       service = subject
 
       expect(service.title).to eq('custom title')
       expect(service.description).to eq('custom description')
     end
+
+    it 'stores data in data_fields correcty' do
+      service = subject
+
+      expect(service.jira_tracker_data.url).to eq(url)
+      expect(service.jira_tracker_data.api_url).to eq(api_url)
+      expect(service.jira_tracker_data.username).to eq(username)
+      expect(service.jira_tracker_data.password).to eq(password)
+      expect(service.jira_tracker_data.jira_issue_transition_id).to eq(transition_id)
+    end
   end
 
+  # we need to make sure we are able to read both from properties and jira_tracker_data table
+  # TODO: change this as part of #63084
   context 'overriding properties' do
-    let(:url) { 'http://issue_tracker.example.com' }
     let(:access_params) do
-      { url: url, username: 'username', password: 'password' }
+      { url: url, api_url: api_url, username: username, password: password,
+        jira_issue_transition_id: transition_id }
+    end
+
+    shared_examples 'handles jira fields' do
+      let(:data_params) do
+        {
+          url: url, api_url: api_url,
+          username: username, password: password,
+          jira_issue_transition_id: transition_id,
+          title: title, description: description
+        }
+      end
+
+      context 'reading data' do
+        it 'reads data correctly' do
+          expect(service.url).to eq(url)
+          expect(service.api_url).to eq(api_url)
+          expect(service.username).to eq(username)
+          expect(service.password).to eq(password)
+          expect(service.jira_issue_transition_id).to eq(transition_id)
+        end
+      end
+
+      context '#update' do
+        context 'basic update' do
+          let(:new_username) { 'new_username' }
+          let(:new_url) { 'http://jira-new.example.com' }
+
+          before do
+            service.update(username: new_username, url: new_url)
+          end
+
+          it 'leaves properties field emtpy' do
+            # expect(service.reload.properties).to be_empty
+          end
+
+          it 'stores updated data in jira_tracker_data table' do
+            data = service.jira_tracker_data.reload
+
+            expect(data.url).to eq(new_url)
+            expect(data.api_url).to eq(api_url)
+            expect(data.username).to eq(new_username)
+            expect(data.password).to eq(password)
+            expect(data.jira_issue_transition_id).to eq(transition_id)
+          end
+        end
+
+        context 'stored password invalidation' do
+          context 'when a password was previously set' do
+            context 'when only web url present' do
+              let(:data_params) do
+                {
+                  url: url, api_url: nil,
+                  username: username, password: password,
+                  jira_issue_transition_id: transition_id
+                }
+              end
+
+              it 'resets password if url changed' do
+                service
+                service.url = 'http://jira_edited.example.com'
+                service.save
+
+                expect(service.reload.url).to eq('http://jira_edited.example.com')
+                expect(service.password).to be_nil
+              end
+
+              it 'resets password if url not changed but api url added' do
+                service.api_url = 'http://jira_edited.example.com/rest/api/2'
+                service.save
+
+                expect(service.reload.api_url).to eq('http://jira_edited.example.com/rest/api/2')
+                expect(service.password).to be_nil
+              end
+
+              it 'does not reset password if new url is set together with password, even if it\'s the same password' do
+                service.url = 'http://jira_edited.example.com'
+                service.password = password
+                service.save
+
+                expect(service.password).to eq(password)
+                expect(service.url).to eq('http://jira_edited.example.com')
+              end
+
+              it 'resets password if url changed, even if setter called multiple times' do
+                service.url = 'http://jira1.example.com/rest/api/2'
+                service.url = 'http://jira1.example.com/rest/api/2'
+                service.save
+
+                expect(service.password).to be_nil
+              end
+
+              it 'does not reset password if username changed' do
+                service.username = 'some_name'
+                service.save
+
+                expect(service.reload.password).to eq(password)
+              end
+            end
+
+            context 'when both web and api url present' do
+              let(:data_params) do
+                {
+                  url: url, api_url: 'http://jira.example.com/rest/api/2',
+                  username: username, password: password,
+                  jira_issue_transition_id: transition_id
+                }
+              end
+
+              it 'resets password if api url changed' do
+                service.api_url = 'http://jira_edited.example.com/rest/api/2'
+                service.save
+
+                expect(service.password).to be_nil
+              end
+
+              it 'does not reset password if url changed' do
+                service.url = 'http://jira_edited.example.com'
+                service.save
+
+                expect(service.password).to eq(password)
+              end
+
+              it 'resets password if api url set to empty' do
+                service.update(api_url: '')
+
+                expect(service.reload.password).to be_nil
+              end
+            end
+          end
+
+          context 'when no password was previously set' do
+            let(:data_params) do
+              {
+                url: url, username: username
+              }
+            end
+
+            it 'saves password if new url is set together with password' do
+              service.url = 'http://jira_edited.example.com/rest/api/2'
+              service.password = 'password'
+              service.save
+              expect(service.reload.password).to eq('password')
+              expect(service.reload.url).to eq('http://jira_edited.example.com/rest/api/2')
+            end
+          end
+        end
+      end
     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) do
-        create(:jira_service, :without_properties_callback, properties: properties)
+      let(:properties) { data_params.merge(title: title, description: description) }
+      let!(:service) do
+        create(:jira_service, :without_properties_callback, properties: properties, create_data: false)
       end
 
       include_examples 'issue tracker fields'
+      include_examples 'handles jira fields'
     end
 
     context 'when data are stored in separated fields' do
       let(:service) do
-        create(:jira_service, title: title, description: description, properties: access_params)
+        create(:jira_service, data_params.merge(properties: {}))
       end
 
       include_examples 'issue tracker fields'
+      include_examples 'handles jira 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(:properties) { data_params.merge(title: 'wrong title', description: 'wrong description') }
       let(:service) do
-        create(:jira_service, :without_properties_callback, title: title, description: description, properties: properties)
+        create(:jira_service, :without_properties_callback, data_params.merge(properties: {}))
       end
 
       include_examples 'issue tracker fields'
+      include_examples 'handles jira fields'
     end
 
     context 'when no title & description are set' do
@@ -338,111 +518,6 @@ describe JiraService do
     end
   end
 
-  describe 'Stored password invalidation' do
-    let(:project) { create(:project) }
-
-    context 'when a password was previously set' do
-      before do
-        @jira_service = described_class.create!(
-          project: project,
-          properties: {
-            url: 'http://jira.example.com/web',
-            username: 'mic',
-            password: 'password'
-          }
-        )
-      end
-
-      context 'when only web url present' do
-        it 'reset password if url changed' do
-          @jira_service.url = 'http://jira_edited.example.com/rest/api/2'
-          @jira_service.save
-
-          expect(@jira_service.password).to be_nil
-        end
-
-        it 'reset password if url not changed but api url added' do
-          @jira_service.api_url = 'http://jira_edited.example.com/rest/api/2'
-          @jira_service.save
-
-          expect(@jira_service.password).to be_nil
-        end
-      end
-
-      context 'when both web and api url present' do
-        before do
-          @jira_service.api_url = 'http://jira.example.com/rest/api/2'
-          @jira_service.password = 'password'
-
-          @jira_service.save
-        end
-        it 'reset password if api url changed' do
-          @jira_service.api_url = 'http://jira_edited.example.com/rest/api/2'
-          @jira_service.save
-
-          expect(@jira_service.password).to be_nil
-        end
-
-        it 'does not reset password if url changed' do
-          @jira_service.url = 'http://jira_edited.example.com/rweb'
-          @jira_service.save
-
-          expect(@jira_service.password).to eq('password')
-        end
-
-        it 'reset password if api url set to empty' do
-          @jira_service.api_url = ''
-          @jira_service.save
-
-          expect(@jira_service.password).to be_nil
-        end
-      end
-
-      it 'does not reset password if username changed' do
-        @jira_service.username = 'some_name'
-        @jira_service.save
-
-        expect(@jira_service.password).to eq('password')
-      end
-
-      it 'does not reset password if new url is set together with password, even if it\'s the same password' do
-        @jira_service.url = 'http://jira_edited.example.com/rest/api/2'
-        @jira_service.password = 'password'
-        @jira_service.save
-
-        expect(@jira_service.password).to eq('password')
-        expect(@jira_service.url).to eq('http://jira_edited.example.com/rest/api/2')
-      end
-
-      it 'resets password if url changed, even if setter called multiple times' do
-        @jira_service.url = 'http://jira1.example.com/rest/api/2'
-        @jira_service.url = 'http://jira1.example.com/rest/api/2'
-        @jira_service.save
-        expect(@jira_service.password).to be_nil
-      end
-    end
-
-    context 'when no password was previously set' do
-      before do
-        @jira_service = described_class.create(
-          project: project,
-          properties: {
-            url: 'http://jira.example.com/rest/api/2',
-            username: 'mic'
-          }
-        )
-      end
-
-      it 'saves password if new url is set together with password' do
-        @jira_service.url = 'http://jira_edited.example.com/rest/api/2'
-        @jira_service.password = 'password'
-        @jira_service.save
-        expect(@jira_service.password).to eq('password')
-        expect(@jira_service.url).to eq('http://jira_edited.example.com/rest/api/2')
-      end
-    end
-  end
-
   describe 'description and title' do
     let(:title) { 'Jira One' }
     let(:description) { 'Jira One issue tracker' }
@@ -467,7 +542,7 @@ describe JiraService do
 
     context 'when it is set in properties' do
       it 'values from properties are returned' do
-        service = create(:jira_service, properties: properties)
+        service = create(:jira_service, :without_properties_callback, properties: properties)
 
         expect(service.title).to eq(title)
         expect(service.description).to eq(description)
@@ -530,8 +605,8 @@ describe JiraService do
         project = create(:project)
         service = project.create_jira_service(active: true)
 
-        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.url).to eq('http://jira.sample/projects/project_a')
+        expect(service.api_url).to eq('http://jira.sample/api')
       end
     end
 
diff --git a/spec/models/project_services/redmine_service_spec.rb b/spec/models/project_services/redmine_service_spec.rb
index c1ee6546b12..310ec1450eb 100644
--- a/spec/models/project_services/redmine_service_spec.rb
+++ b/spec/models/project_services/redmine_service_spec.rb
@@ -9,28 +9,7 @@ describe RedmineService do
   end
 
   describe 'Validations' do
-    context 'when service is active' do
-      before do
-        subject.active = true
-      end
-
-      it { is_expected.to validate_presence_of(:project_url) }
-      it { is_expected.to validate_presence_of(:issues_url) }
-      it { is_expected.to validate_presence_of(:new_issue_url) }
-      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
-    end
-
-    context 'when service is inactive' do
-      before do
-        subject.active = false
-      end
-
-      it { is_expected.not_to validate_presence_of(:project_url) }
-      it { is_expected.not_to validate_presence_of(:issues_url) }
-      it { is_expected.not_to validate_presence_of(:new_issue_url) }
-    end
+    # TODO validate data fields
   end
 
   describe '.reference_pattern' do
diff --git a/spec/models/project_services/youtrack_service_spec.rb b/spec/models/project_services/youtrack_service_spec.rb
index c48bf487af0..9366ce16644 100644
--- a/spec/models/project_services/youtrack_service_spec.rb
+++ b/spec/models/project_services/youtrack_service_spec.rb
@@ -9,25 +9,7 @@ describe YoutrackService do
   end
 
   describe 'Validations' do
-    context 'when service is active' do
-      before do
-        subject.active = true
-      end
-
-      it { is_expected.to validate_presence_of(:project_url) }
-      it { is_expected.to validate_presence_of(:issues_url) }
-      it_behaves_like 'issue tracker service URL attribute', :project_url
-      it_behaves_like 'issue tracker service URL attribute', :issues_url
-    end
-
-    context 'when service is inactive' do
-      before do
-        subject.active = false
-      end
-
-      it { is_expected.not_to validate_presence_of(:project_url) }
-      it { is_expected.not_to validate_presence_of(:issues_url) }
-    end
+    # TODO validate data fields
   end
 
   describe '.reference_pattern' do
diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb
index 0797b9a9d83..d96e1398677 100644
--- a/spec/models/service_spec.rb
+++ b/spec/models/service_spec.rb
@@ -257,8 +257,8 @@ describe Service do
       expect(service.title).to eq('random title')
     end
 
-    it 'creates the properties' do
-      expect(service.properties).to eq({ "project_url" => "http://gitlab.example.com" })
+    it 'sets data correctly' do
+      expect(service.data_fields.project_url).to eq('http://gitlab.example.com')
     end
   end
 
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index 910fe3b50b7..1f2a2e161a1 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -824,7 +824,7 @@ describe SystemNoteService do
     let(:jira_tracker)    { project.jira_service }
     let(:commit)          { project.commit }
     let(:comment_url)     { jira_api_comment_url(jira_issue.id) }
-    let(:success_message) { "SUCCESS: Successfully posted to http://jira.example.net." }
+    let(:success_message) { "SUCCESS: Successfully posted to https://jira-api.example.com." }
 
     before do
       stub_jira_urls(jira_issue.id)
diff --git a/spec/support/helpers/jira_service_helper.rb b/spec/support/helpers/jira_service_helper.rb
index 57c33c81ea3..151cd7054e7 100644
--- a/spec/support/helpers/jira_service_helper.rb
+++ b/spec/support/helpers/jira_service_helper.rb
@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 module JiraServiceHelper
-  JIRA_URL = "http://jira.example.net".freeze
+  JIRA_URL = "https://jira-api.example.com".freeze
   JIRA_API = JIRA_URL + "/rest/api/2"
 
   def jira_service_settings
@@ -10,7 +10,6 @@ module JiraServiceHelper
       url: JIRA_URL,
       username: 'jira-user',
       password: 'my-secret-password',
-      project_key: "JIRA",
       jira_issue_transition_id: '1'
     }