Add API URL to JIRA settings31448-jira-urls

6 files changed, 156 insertions, 54 deletions

diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb
index a91a986e195..fe869623833 100644
--- a/app/models/project_services/jira_service.rb
+++ b/app/models/project_services/jira_service.rb
@@ -2,9 +2,10 @@ class JiraService < IssueTrackerService
   include Gitlab::Routing.url_helpers
 
   validates :url, url: true, presence: true, if: :activated?
+  validates :api_url, url: true, allow_blank: true
   validates :project_key, presence: true, if: :activated?
 
-  prop_accessor :username, :password, :url, :project_key,
+  prop_accessor :username, :password, :url, :api_url, :project_key,
                 :jira_issue_transition_id, :title, :description
 
   before_update :reset_password
@@ -25,20 +26,18 @@ class JiraService < IssueTrackerService
     super do
       self.properties = {
         title: issues_tracker['title'],
-        url: issues_tracker['url']
+        url: issues_tracker['url'],
+        api_url: issues_tracker['api_url']
       }
     end
   end
 
   def reset_password
-    # don't reset the password if a new one is provided
-    if url_changed? && !password_touched?
-      self.password = nil
-    end
+    self.password = nil if reset_password?
   end
 
   def options
-    url = URI.parse(self.url)
+    url = URI.parse(client_url)
 
     {
       username: self.username,
@@ -87,7 +86,8 @@ class JiraService < IssueTrackerService
 
   def fields
     [
-      { type: 'text', name: 'url', title: 'URL', placeholder: 'https://jira.example.com' },
+      { type: 'text', name: 'url', title: 'Web URL', placeholder: 'https://jira.example.com' },
+      { type: 'text', name: 'api_url', title: 'JIRA API URL', placeholder: 'If different from Web URL' },
       { type: 'text', name: 'project_key', placeholder: 'Project Key' },
       { type: 'text', name: 'username', placeholder: '' },
       { type: 'password', name: 'password', placeholder: '' },
@@ -186,7 +186,7 @@ class JiraService < IssueTrackerService
   end
 
   def test_settings
-    return unless url.present?
+    return unless client_url.present?
     # Test settings by getting the project
     jira_request { jira_project.present? }
   end
@@ -236,13 +236,13 @@ class JiraService < IssueTrackerService
   end
 
   def send_message(issue, message, remote_link_props)
-    return unless url.present?
+    return unless client_url.present?
 
     jira_request do
       if issue.comments.build.save!(body: message)
         remote_link = issue.remotelink.build
         remote_link.save!(remote_link_props)
-        result_message = "#{self.class.name} SUCCESS: Successfully posted to #{url}."
+        result_message = "#{self.class.name} SUCCESS: Successfully posted to #{client_url}."
       end
 
       Rails.logger.info(result_message)
@@ -295,7 +295,20 @@ class JiraService < IssueTrackerService
     yield
 
   rescue Timeout::Error, Errno::EINVAL, Errno::ECONNRESET, Errno::ECONNREFUSED, URI::InvalidURIError, JIRA::HTTPError, OpenSSL::SSL::SSLError => e
-    Rails.logger.info "#{self.class.name} Send message ERROR: #{url} - #{e.message}"
+    Rails.logger.info "#{self.class.name} Send message ERROR: #{client_url} - #{e.message}"
     nil
   end
+
+  def client_url
+    api_url.present? ? api_url : url
+  end
+
+  def reset_password?
+    # don't reset the password if a new one is provided
+    return false if password_touched?
+    return true if api_url_changed?
+    return false if api_url.present?
+
+    url_changed?
+  end
 end
diff --git a/changelogs/unreleased/31448-jira-urls.yml b/changelogs/unreleased/31448-jira-urls.yml
new file mode 100644
index 00000000000..d0e39f61b55
--- /dev/null
+++ b/changelogs/unreleased/31448-jira-urls.yml
@@ -0,0 +1,4 @@
+---
+title: Add API URL to JIRA settings
+merge_request:
+author:
diff --git a/doc/user/project/integrations/jira.md b/doc/user/project/integrations/jira.md
index f611029afdc..a048260b033 100644
--- a/doc/user/project/integrations/jira.md
+++ b/doc/user/project/integrations/jira.md
@@ -97,7 +97,8 @@ in the table below.
 
 | Field | Description |
 | ----- | ----------- |
-| `URL` | The base URL to the JIRA project which is being linked to this GitLab project. E.g., `https://jira.example.com`. |
+| `Web URL` | The base URL to the JIRA instance web interface which is being linked to this GitLab project. E.g., `https://jira.example.com`. |
+| `JIRA API URL` | The base URL to the JIRA instance API. Web URL value will be used if not set. E.g., `https://jira-api.example.com`. |
 | `Project key` | The short identifier for your JIRA project, all uppercase, e.g., `PROJ`. |
 | `Username` | The user name created in [configuring JIRA step](#configuring-jira). |
 | `Password` |The password of the user created in [configuring JIRA step](#configuring-jira). |
diff --git a/features/steps/project/services.rb b/features/steps/project/services.rb
index 3c0d987e403..66368a159ec 100644
--- a/features/steps/project/services.rb
+++ b/features/steps/project/services.rb
@@ -178,7 +178,8 @@ class Spinach::Features::ProjectServices < Spinach::FeatureSteps
   end
 
   step 'I fill jira settings' do
-    fill_in 'URL', with: 'http://jira.example'
+    fill_in 'Web URL', with: 'http://jira.example'
+    fill_in 'JIRA API URL', with: 'http://jira.example/api'
     fill_in 'Username', with: 'gitlab'
     fill_in 'Password', with: 'gitlab'
     fill_in 'Project Key', with: 'GITLAB'
@@ -186,7 +187,8 @@ class Spinach::Features::ProjectServices < Spinach::FeatureSteps
   end
 
   step 'I should see jira service settings saved' do
-    expect(find_field('URL').value).to eq 'http://jira.example'
+    expect(find_field('Web URL').value).to eq 'http://jira.example'
+    expect(find_field('JIRA API URL').value).to eq 'http://jira.example/api'
     expect(find_field('Username').value).to eq 'gitlab'
     expect(find_field('Project Key').value).to eq 'GITLAB'
   end
diff --git a/lib/api/services.rb b/lib/api/services.rb
index cb07df9e249..47bd9940f77 100644
--- a/lib/api/services.rb
+++ b/lib/api/services.rb
@@ -304,7 +304,13 @@ module API
           required: true,
           name: :url,
           type: String,
-          desc: 'The URL to the JIRA project which is being linked to this GitLab project, e.g., https://jira.example.com'
+          desc: 'The base URL to the JIRA instance web interface which is being linked to this GitLab project. E.g., https://jira.example.com'
+        },
+        {
+          required: false,
+          name: :api_url,
+          type: String,
+          desc: 'The base URL to the JIRA instance API. Web URL value will be used if not set. E.g., https://jira-api.example.com'
         },
         {
           required: true,
diff --git a/spec/models/project_services/jira_service_spec.rb b/spec/models/project_services/jira_service_spec.rb
index 4bca0229e7a..349067e73ab 100644
--- a/spec/models/project_services/jira_service_spec.rb
+++ b/spec/models/project_services/jira_service_spec.rb
@@ -22,6 +22,42 @@ describe JiraService, models: true do
 
       it { is_expected.not_to validate_presence_of(:url) }
     end
+
+    context 'validating urls' do
+      let(:service) do
+        described_class.new(
+          project: create(:empty_project),
+          active: true,
+          username: 'username',
+          password: 'test',
+          project_key: 'TEST',
+          jira_issue_transition_id: 24,
+          url: 'http://jira.test.com'
+        )
+      end
+
+      it 'is valid when all fields have required values' do
+        expect(service).to be_valid
+      end
+
+      it 'is not valid when url is not a valid url' do
+        service.url = 'not valid'
+
+        expect(service).not_to be_valid
+      end
+
+      it 'is not valid when api url is not a valid url' do
+        service.api_url = 'not valid'
+
+        expect(service).not_to be_valid
+      end
+
+      it 'is valid when api url is a valid url' do
+        service.api_url = 'http://jira.test.com/api'
+
+        expect(service).to be_valid
+      end
+    end
   end
 
   describe '#reference_pattern' do
@@ -187,22 +223,29 @@ describe JiraService, models: true do
   describe '#test_settings' do
     let(:jira_service) do
       described_class.new(
+        project: create(:project),
         url: 'http://jira.example.com',
-        username: 'gitlab_jira_username',
-        password: 'gitlab_jira_password',
+        username: 'jira_username',
+        password: 'jira_password',
         project_key: 'GitLabProject'
       )
     end
-    let(:project_url) { 'http://gitlab_jira_username:gitlab_jira_password@jira.example.com/rest/api/2/project/GitLabProject' }
 
-    before do
+    def test_settings(api_url)
+      project_url = "http://jira_username:jira_password@#{api_url}/rest/api/2/project/GitLabProject"
+
       WebMock.stub_request(:get, project_url)
-    end
 
-    it 'tries to get JIRA project' do
       jira_service.test_settings
+    end
+
+    it 'tries to get JIRA project with URL when API URL not set' do
+      test_settings('jira.example.com')
+    end
 
-      expect(WebMock).to have_requested(:get, project_url)
+    it 'tries to get JIRA project with API URL if set' do
+      jira_service.update(api_url: 'http://jira.api.com')
+      test_settings('jira.api.com')
     end
   end
 
@@ -214,34 +257,75 @@ describe JiraService, models: true do
         @jira_service = JiraService.create!(
           project: project,
           properties: {
-            url: 'http://jira.example.com/rest/api/2',
+            url: 'http://jira.example.com/web',
             username: 'mic',
             password: "password"
           }
         )
       end
 
-      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
+      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
 
-      it "does not reset password if username changed" do
-        @jira_service.username = "some_name"
+      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 ""' 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")
+
+        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
+      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")
+
+        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
+      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
@@ -249,7 +333,7 @@ describe JiraService, models: true do
       end
     end
 
-    context "when no password was previously set" do
+    context 'when no password was previously set' do
       before do
         @jira_service = JiraService.create(
           project: project,
@@ -260,26 +344,16 @@ describe JiraService, models: true do
         )
       end
 
-      it "saves password if new url is set together with password" do
+      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")
+        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 "Validations" do
-    context "active" do
-      before do
-        subject.active = true
-      end
-
-      it { is_expected.to validate_presence_of :url }
-    end
-  end
-
   describe 'description and title' do
     let(:project) { create(:empty_project) }
 
@@ -321,9 +395,10 @@ describe JiraService, models: true do
     context 'when gitlab.yml was initialized' do
       before do
         settings = {
-          "jira" => {
-            "title" => "Jira",
-            "url" => "http://jira.sample/projects/project_a"
+          '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)
@@ -335,8 +410,9 @@ describe JiraService, models: true do
       end
 
       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['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')
       end
     end
   end