Merge branch 'cernvcs/gitlab-ee-feature/jira_api_path'

CHANGELOG-EE

@@ -2,6 +2,7 @@ v 8.2.0
   - Invalidate stored jira password if the endpoint URL is changed
   - Fix: Page is not reloaded periodically to check if rebase is finished
   - When someone as marked as a required approver for a merge request, an email should be sent
+  - Allow configuring the Jira API path (Alex Lossent)
 
 v 8.1.0 (unreleased)
   - added an issues template (Hannes Rosenögger)

app/controllers/projects/services_controller.rb

 class Projects::ServicesController < Projects::ApplicationController
-  ALLOWED_PARAMS = [:title, :token, :type, :active, :api_key, :api_version, :subdomain,
+  ALLOWED_PARAMS = [:title, :token, :type, :active, :api_key, :api_url, :api_version, :subdomain,
                     :room, :recipients, :project_url, :webhook,
                     :user_key, :device, :priority, :sound, :bamboo_url, :username, :password,
                     :build_key, :server, :teamcity_url, :drone_url, :build_type,

app/models/project_services/jira_service.rb

@@ -22,15 +22,18 @@ class JiraService < IssueTrackerService
   include HTTParty
   include Gitlab::Application.routes.url_helpers
 
-  prop_accessor :username, :password, :api_version, :jira_issue_transition_id,
+  DEFAULT_API_VERSION = 2
+
+  prop_accessor :username, :password, :api_url, :jira_issue_transition_id,
                 :title, :description, :project_url, :issues_url, :new_issue_url
 
-  before_validation :set_api_version, :set_jira_issue_transition_id
+  before_validation :set_api_url, :set_jira_issue_transition_id
 
   before_update :reset_password
 
   def reset_password
-    if project_url_changed? && !password_touched?
+    # don't reset the password if a new one is provided
+    if api_url_changed? && !password_touched?
       self.password = nil
     end
   end
@@ -69,9 +72,9 @@ class JiraService < IssueTrackerService
 
   def fields
     super.push(
+      { type: 'text', name: 'api_url', placeholder: 'https://jira.example.com/rest/api/2' },
       { type: 'text', name: 'username', placeholder: '' },
       { type: 'password', name: 'password', placeholder: '' },
-      { type: 'text', name: 'api_version', placeholder: '2' },
       { type: 'text', name: 'jira_issue_transition_id', placeholder: '2' }
     )
   end
@@ -118,7 +121,7 @@ class JiraService < IssueTrackerService
 
   def test_settings
     result = JiraService.get(
-      jira_project_url,
+      jira_api_test_url,
       headers: {
         'Content-Type' => 'application/json',
         'Authorization' => "Basic #{auth}"
@@ -127,22 +130,31 @@ class JiraService < IssueTrackerService
 
     case result.code
     when 201, 200
-      Rails.logger.info("#{self.class.name} SUCCESS #{result.code}: Sucessfully connected to #{server_url}.")
+      Rails.logger.info("#{self.class.name} SUCCESS #{result.code}: Sucessfully connected to #{api_url}.")
       true
     else
       Rails.logger.info("#{self.class.name} ERROR #{result.code}: #{result.parsed_response}")
       false
     end
   rescue Errno::ECONNREFUSED => e
-    Rails.logger.info "#{self.class.name} ERROR: #{e.message}. Hostname: #{server_url}."
+    Rails.logger.info "#{self.class.name} ERROR: #{e.message}. API URL: #{api_url}."
     false
   end
 
   private
 
+  def build_api_url_from_project_url
+    server = URI(project_url)
+    default_ports = [["http",80],["https",443]].include?([server.scheme,server.port])
+    server_url = "#{server.scheme}://#{server.host}"
+    server_url.concat(":#{server.port}") unless default_ports
+    "#{server_url}/rest/api/#{DEFAULT_API_VERSION}"
+  rescue
+    "" # looks like project URL was not valid
+  end
 
-  def set_api_version
-    self.api_version ||= "2"
+  def set_api_url
+    self.api_url = build_api_url_from_project_url if self.api_url.blank?
   end
 
   def set_jira_issue_transition_id
@@ -241,14 +253,6 @@ class JiraService < IssueTrackerService
     false
   end
 
-  def server_url
-    server = URI(project_url)
-    default_ports = [80, 443].include?(server.port)
-    server_url = "#{server.scheme}://#{server.host}"
-    server_url.concat(":#{server.port}") unless default_ports
-    server_url
-  end
-
   def resource_url(resource)
     "#{Settings.gitlab['url'].chomp("/")}#{resource}"
   end
@@ -268,14 +272,14 @@ class JiraService < IssueTrackerService
   end
 
   def close_issue_url(issue_name)
-    "#{server_url}/rest/api/#{self.api_version}/issue/#{issue_name}/transitions"
+    "#{self.api_url}/issue/#{issue_name}/transitions"
   end
 
   def comment_url(issue_name)
-    "#{server_url}/rest/api/#{self.api_version}/issue/#{issue_name}/comment"
+    "#{self.api_url}/issue/#{issue_name}/comment"
   end
 
-  def jira_project_url
-    "#{server_url}/rest/api/#{self.api_version}/project"
+  def jira_api_test_url
+    "#{self.api_url}/myself"
   end
 end

db/migrate/20151012173029_set_jira_service_api_url.rb

+class SetJiraServiceApiUrl < ActiveRecord::Migration
+  # This migration can be performed online without errors, but some Jira API calls may be missed
+  # when doing so because api_url is not yet available.
+
+  def build_api_url_from_project_url(project_url, api_version)
+    # this is the exact logic previously used to build the Jira API URL from project_url
+    server = URI(project_url)
+    default_ports = [80, 443].include?(server.port)
+    server_url = "#{server.scheme}://#{server.host}"
+    server_url.concat(":#{server.port}") unless default_ports
+    "#{server_url}/rest/api/#{api_version}"
+  end
+
+  def get_api_version_from_api_url(api_url)
+    match = /\/rest\/api\/(?<api_version>\w+)$/.match(api_url)
+    match && match['api_version']
+  end
+
+  def change
+    reversible do |dir|
+      select_all("SELECT id, properties FROM services WHERE services.type IN ('JiraService')").each do |jira_service|
+        id = jira_service["id"]
+        properties = JSON.parse(jira_service["properties"])
+        properties_was = properties.clone
+
+        dir.up do
+          # remove api_version and set api_url
+          if properties['api_version'].present? && properties['project_url'].present?
+            begin
+              properties['api_url'] ||= build_api_url_from_project_url(properties['project_url'], properties['api_version'])
+            rescue
+              # looks like project_url was not a valid URL. Do nothing.
+            end
+          end
+          properties.delete('api_version') if properties.include?('api_version')
+        end
+
+        dir.down do
+          # remove api_url and set api_version (default to '2')
+          properties['api_version'] ||= get_api_version_from_api_url(properties['api_url']) || '2'
+          properties.delete('api_url') if properties.include?('api_url')
+        end
+
+        if properties != properties_was
+          execute("UPDATE services SET properties = '#{quote_string(properties.to_json)}' WHERE id = #{id}")
+        end
+      end
+    end
+  end
+end

doc/integration/jira.md

@@ -74,9 +74,9 @@ Fill in the required details on the page:
 * `project url` The URL to the JIRA project which is being linked to this GitLab project.
 * `issues url` The URL to the JIRA project issues overview for the project that is linked to this GitLab project.
 * `new issue url` This is the URL to create a new issue in JIRA for the project linked to this GitLab project.
+* `api url` The base URL of the JIRA API. It may be omitted, in which case GitLab will automatically use API version `2` based on the `project url`, i.e. `https://jira.example.com/rest/api/2`.
 * `username` The username of the user created in [configuring JIRA step](#configuring-jira).
 * `password` The password of the user created in [configuring JIRA step](#configuring-jira).
-* `api version` The version of the JIRA API. By default, version `2` is used.
 * `Jira issue transition` This is the id of a transition that moves issues to a closed state. You can find this number under [JIRA workflow administration, see screenshot](jira_workflow_screenshot.png).  By default, this id is `2`. (In the example image, this is `2` as well)
 
 After saving the configuration, your GitLab project will be able to interact with the linked JIRA project.

features/steps/project/services.rb

@@ -191,14 +191,14 @@ class Spinach::Features::ProjectServices < Spinach::FeatureSteps
     fill_in 'Project url', with: 'http://jira.example'
     fill_in 'Username', with: 'gitlab'
     fill_in 'Password', with: 'gitlab'
-    fill_in 'Api version', with: '2'
+    fill_in 'Api url', with: 'http://jira.example/rest/api/2'
     click_button 'Save'
   end
 
   step 'I should see jira service settings saved' do
     expect(find_field('Project url').value).to eq 'http://jira.example'
     expect(find_field('Username').value).to eq 'gitlab'
-    expect(find_field('Api version').value).to eq '2'
+    expect(find_field('Api url').value).to eq 'http://jira.example/rest/api/2'
   end
 
   step 'I click Atlassian Bamboo CI service link' do

spec/models/project_services/jira_service_spec.rb

@@ -40,9 +40,9 @@ describe JiraService do
         service_hook: true,
         project_url: 'http://jira.example.com',
         username: 'gitlab_jira_username',
-        password: 'gitlab_jira_password',
-        api_version: '2'
+        password: 'gitlab_jira_password'
       )
+      @jira_service.save # will build API URL, as api_url was not specified above
       @sample_data = Gitlab::PushDataBuilder.build_sample(project, user)
       # https://github.com/bblimke/webmock#request-with-basic-authentication
       @api_url = 'http://gitlab_jira_username:gitlab_jira_password@jira.example.com/rest/api/2/issue/JIRA-123/transitions'
@@ -66,6 +66,72 @@ 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 = JiraService.create(
+          project: create(:project),
+          properties: {
+            api_url: 'http://jira.example.com/rest/api/2',
+            username: 'mic',
+            password: "password"
+          }
+        )
+      end
+  
+      it "reset password if 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 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.api_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.api_url).to eq("http://jira_edited.example.com/rest/api/2")
+      end
+
+      it "should reset password if url changed, even if setter called multiple times" do
+        @jira_service.api_url = 'http://jira1.example.com/rest/api/2'
+        @jira_service.api_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 = JiraService.create(
+          project: create(:project),
+          properties: {
+            api_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.api_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.api_url).to eq("http://jira_edited.example.com/rest/api/2")
+      end
+
+    end
+  end
+
+
   describe "Validations" do
     context "active" do
       before do
@@ -140,69 +206,4 @@ describe JiraService do
       end
     end
   end
-
-  describe "Execute" do
-    let(:user)    { create(:user) }
-    let(:project) { create(:project) }
-
-    context "when a password was previously set" do
-      before do
-        @service = JiraService.create(
-          project: create(:project),
-          properties: {
-            project_url: 'http://gitlab.com',
-            username: 'mic',
-            password: "password"
-          }
-        )
-      end
-  
-      it "reset password if url changed" do
-        @service.project_url = 'http://gitlab1.com'
-        @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.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
-        @service.project_url = 'http://gitlab_edited.com'
-        @service.password = 'password'
-        @service.save
-        expect(@service.password).to eq("password")
-        expect(@service.project_url).to eq("http://gitlab_edited.com")
-      end
-
-      it "should reset password if url changed, even if setter called multiple times" do
-        @service.project_url = 'http://gitlab1.com'
-        @service.project_url = 'http://gitlab1.com'
-        @service.save
-        expect(@service.password).to be_nil
-      end
-    end
-    
-    context "when no password was previously set" do
-      before do
-        @service = JiraService.create(
-          project: create(:project),
-          properties: {
-            project_url: 'http://gitlab.com',
-            username: 'mic'
-          }
-        )
-      end
-
-      it "saves password if new url is set together with password" do
-        @service.project_url = 'http://gitlab_edited.com'
-        @service.password = 'password'
-        @service.save
-        expect(@service.password).to eq("password")
-        expect(@service.project_url).to eq("http://gitlab_edited.com")
-      end
-    end
-  end
 end

spec/services/git_push_service_spec.rb

@@ -278,7 +278,7 @@ describe GitPushService do
         WebMock.stub_request(:post, jira_api_transition_url)
         WebMock.stub_request(:post, jira_api_comment_url)
         WebMock.stub_request(:get, jira_api_comment_url).to_return(body: jira_issue_comments)
-        WebMock.stub_request(:get, jira_api_project_url)
+        WebMock.stub_request(:get, jira_api_test_url)
 
         allow(closing_commit).to receive_messages({
           issue_closing_regex: Regexp.new(Gitlab.config.gitlab.issue_closing_pattern),

spec/support/jira_service_helper.rb

@@ -5,7 +5,8 @@ module JiraServiceHelper
       "title"=>"JIRA tracker",
       "project_url"=>"http://jira.example/issues/?jql=project=A",
       "issues_url"=>"http://jira.example/browse/JIRA-1",
-      "new_issue_url"=>"http://jira.example/secure/CreateIssue.jspa"
+      "new_issue_url"=>"http://jira.example/secure/CreateIssue.jspa",
+      "api_url"=>"http://jira.example/rest/api/2"
     }
 
     jira_tracker.update_attributes(properties: properties, active: true)
@@ -60,7 +61,7 @@ module JiraServiceHelper
     'http://jira.example/rest/api/2/issue/JIRA-1/transitions'
   end
 
-  def jira_api_project_url
-    'http://jira.example/rest/api/2/project'
+  def jira_api_test_url
+    'http://jira.example/rest/api/2/myself'
   end
 end