Fix the trailing slash bug for BambooService too

Signed-off-by: Rémy Coutable <remy@rymai.me>

CHANGELOG

@@ -13,6 +13,7 @@ v 8.7.0 (unreleased)
   - Expose label description in API (Mariusz Jachimowicz)
   - Allow back dating on issues when created through the API
   - Fix Error 500 after renaming a project path (Stan Hu)
+  - Fix a bug whith trailing slash in teamcity_url & bamboo_url (Charles May)
   - Fix avatar stretching by providing a cropping feature
   - API: Expose `subscribed` for issues and merge requests (Robert Schilling)
   - Allow SAML to handle external users based on user's information !3530
@@ -124,7 +125,6 @@ v 8.6.0
   - HTTP error pages work independently from location and config (Artem Sidorenko)
   - Update `omniauth-saml` to 1.5.0 to allow for custom response attributes to be set
   - Fix avatar stretching by providing a cropping feature (Johann Pardanaud)
-  - Fix a bug whith trailing slash in teamcity_url (Charles May)
   - Don't load all of GitLab in mail_room
   - Memoize @group in Admin::GroupsController (Yatish Mehta)
   - Indicate how much an MR diverged from the target branch (Pierre de La Morinerie)

app/models/project_services/bamboo_service.rb

@@ -82,17 +82,17 @@ class BambooService < CiService
   end
   def build_info(sha)
-    url = URI.parse("#{bamboo_url}/rest/api/latest/result?label=#{sha}")
+    url = URI.join(bamboo_url, "/rest/api/latest/result?label=#{sha}").to_s
     if username.blank? && password.blank?
-      @response = HTTParty.get(parsed_url.to_s, verify: false)
+      @response = HTTParty.get(url, verify: false)
     else
-      get_url = "#{url}&os_authType=basic"
+      url << '&os_authType=basic'
       auth = {
-          username: username,
-          password: password,
+        username: username,
+        password: password
       }
-      @response = HTTParty.get(get_url, verify: false, basic_auth: auth)
+      @response = HTTParty.get(url, verify: false, basic_auth: auth)
     end
   end
@@ -101,11 +101,11 @@ class BambooService < CiService
     if @response.code != 200 || @response['results']['results']['size'] == '0'
       # If actual build link can't be determined, send user to build summary page.
-      "#{bamboo_url}/browse/#{build_key}"
+      URI.join(bamboo_url, "/browse/#{build_key}").to_s
     else
       # If actual build link is available, go to build result page.
       result_key = @response['results']['results']['result']['planResultKey']['key']
-      "#{bamboo_url}/browse/#{result_key}"
+      URI.join(bamboo_url, "/browse/#{result_key}").to_s
     end
   end
@@ -120,11 +120,11 @@ class BambooService < CiService
              end
     if status.include?('Success')
-      'success'
+      :success
     elsif status.include?('Failed')
-      'failed'
+      :failed
     elsif status.include?('Pending')
-      'pending'
+      :pending
     else
       :error
     end
@@ -134,7 +134,7 @@ class BambooService < CiService
     return unless supported_events.include?(data[:object_kind])
     # Bamboo requires a GET and does not take any data.
-    self.class.get("#{bamboo_url}/updateAndBuild.action?buildKey=#{build_key}",
-                   verify: false)
+    url = URI.join(bamboo_url, "/updateAndBuild.action?buildKey=#{build_key}").to_s
+    self.class.get(url, verify: false)
   end
 end

app/models/project_services/teamcity_service.rb

@@ -91,7 +91,7 @@ class TeamcityService < CiService
     ).to_s
     auth = {
       username: username,
-      password: password,
+      password: password
     }
     @response = HTTParty.get(url, verify: false, basic_auth: auth)
   end
@@ -108,7 +108,7 @@ class TeamcityService < CiService
       built_id = @response['build']['id']
       URI.join(
         teamcity_url,
-        "#{teamcity_url}/viewLog.html?buildId=#{built_id}&buildTypeId=#{build_type}"
+        "/viewLog.html?buildId=#{built_id}&buildTypeId=#{build_type}"
       ).to_s
     end
   end
@@ -124,11 +124,11 @@ class TeamcityService < CiService
              end
     if status.include?('SUCCESS')
-      'success'
+      :success
     elsif status.include?('FAILURE')
-      'failed'
+      :failed
     elsif status.include?('Pending')
-      'pending'
+      :pending
     else
       :error
     end
@@ -145,7 +145,7 @@ class TeamcityService < CiService
     branch = Gitlab::Git.ref_name(data[:ref])
     self.class.post(
-      URI.join(teamcity_url, "/httpAuth/app/rest/buildQueue").to_s,
+      URI.join(teamcity_url, '/httpAuth/app/rest/buildQueue').to_s,
       body: "<build branchName=\"#{branch}\">"\
             "<buildType id=\"#{build_type}\"/>"\
             '</build>',

spec/models/project_services/bamboo_service_spec.rb

@@ -41,13 +41,13 @@ describe BambooService, models: true do
           }
         )
       end
-  
+
       it "reset password if url changed" do
         @bamboo_service.bamboo_url = 'http://gitlab1.com'
         @bamboo_service.save
         expect(@bamboo_service.password).to be_nil
       end
-  
+
       it "does not reset password if username changed" do
         @bamboo_service.username = "some_name"
         @bamboo_service.save
@@ -69,7 +69,7 @@ describe BambooService, models: true do
         expect(@bamboo_service.password).to be_nil
       end
     end
-    
+
     context "when no password was previously set" do
       before do
         @bamboo_service = BambooService.create(
@@ -88,7 +88,168 @@ describe BambooService, models: true do
         expect(@bamboo_service.password).to eq("password")
         expect(@bamboo_service.bamboo_url).to eq("http://gitlab_edited.com")
       end
+    end
+  end
+
+  module BambooServiceSpec
+    Response = Struct.new(:code, :data) do
+      def [](key)
+        data[key]
+      end
+    end
+  end
+
+  describe '#build_info' do
+    let(:bamboo_url) { 'http://gitlab.com' }
+    let(:response) { BambooServiceSpec::Response.new(200, {}) }
+    subject do
+      BambooService.create(
+        project: create(:project),
+        properties: {
+          bamboo_url: bamboo_url,
+          username: 'mic',
+          password: 'password'
+        })
+    end
+    before { allow(HTTParty).to receive(:get).and_return(response) }
+
+    context 'when username and password are blank' do
+      subject do
+        BambooService.create(
+          project: create(:project),
+          properties: {
+            bamboo_url: bamboo_url
+          })
+      end
+
+      it { expect(subject.build_info('123')).to eq(response) }
+    end
+
+    context 'when bamboo_url has no trailing slash' do
+      it { expect(subject.build_info('123')).to eq(response) }
+    end
+
+    context 'when bamboo_url has a trailing slash' do
+      let(:bamboo_url) { 'http://gitlab.com/' }
+
+      it { expect(subject.build_info('123')).to eq(response) }
+    end
+  end
+
+  describe '#build_page' do
+    let(:bamboo_url) { 'http://gitlab.com' }
+    let(:response_code) { 200 }
+    let(:response) do
+      BambooServiceSpec::Response.new(response_code, {
+        'results' => {
+          'results' => { 'result' => { 'planResultKey' => { 'key' => '42' } } }
+        }
+      })
+    end
+    subject do
+      BambooService.create(
+        project: create(:project),
+        properties: {
+          bamboo_url: bamboo_url,
+          username: 'mic',
+          password: 'password',
+          build_key: 'foo'
+        })
+    end
+    before { allow(HTTParty).to receive(:get).and_return(response) }
+
+    context 'when bamboo_url has no trailing slash' do
+      it { expect(subject.build_page('123', 'unused')).to eq('http://gitlab.com/browse/42') }
+    end
+
+    context 'when bamboo_url has a trailing slash' do
+      let(:bamboo_url) { 'http://gitlab.com/' }
+
+      it { expect(subject.build_page('123', 'unused')).to eq('http://gitlab.com/browse/42') }
+    end
+
+    context 'when response code is not 200' do
+      let(:response_code) { 500 }
+
+      it { expect(subject.build_page('123', 'unused')).to eq('http://gitlab.com/browse/foo') }
+    end
+
+    context 'when response returns no result' do
+      let(:response) do
+        BambooServiceSpec::Response.new(response_code, {
+          'results' => { 'results' => { 'size' => '0' } }
+        })
+      end
+
+      it { expect(subject.build_page('123', 'unused')).to eq('http://gitlab.com/browse/foo') }
+    end
+  end
+
+  describe '#commit_status' do
+    let(:bamboo_url) { 'http://gitlab.com' }
+    let(:response_code) { 200 }
+    let(:build_state) { 'YAY Success!' }
+    let(:response) do
+      BambooServiceSpec::Response.new(response_code, {
+        'results' => { 'results' => { 'result' => { 'buildState' => build_state } } }
+      })
+    end
+    subject do
+      BambooService.create(
+        project: create(:project),
+        properties: {
+          bamboo_url: bamboo_url,
+          username: 'mic',
+          password: 'password',
+          build_type: 'foo'
+        })
+    end
+    before { allow(HTTParty).to receive(:get).and_return(response) }
+
+    context 'when response code is not 200' do
+      let(:response_code) { 500 }
+
+      it { expect(subject.commit_status('123', 'unused')).to eq(:error) }
+    end
+
+    context 'when response has no results' do
+      let(:response) do
+        BambooServiceSpec::Response.new(response_code, {
+          'results' => { 'results' => { 'size' => '0' } }
+        })
+      end
+      it { expect(subject.commit_status('123', 'unused')).to eq(:pending) }
+    end
+
+    context 'when response code is 404' do
+      let(:response_code) { 404 }
+
+      it { expect(subject.commit_status('123', 'unused')).to eq(:pending) }
+    end
+
+    context 'when response code is 200' do
+      context 'when build status contains Success' do
+        it { expect(subject.commit_status('123', 'unused')).to eq(:success) }
+      end
+
+      context 'when build status contains Failed' do
+        let(:build_state) { 'NO Failed!' }
+
+        it { expect(subject.commit_status('123', 'unused')).to eq(:failed) }
+      end
+
+      context 'when build status contains Failed' do
+        let(:build_state) { 'NO Pending!' }
+
+        it { expect(subject.commit_status('123', 'unused')).to eq(:pending) }
+      end
+
+      context 'when build status contains anything else' do
+        let(:build_state) { 'FOO BAR!' }
+
+        it { expect(subject.commit_status('123', 'unused')).to eq(:error) }
+      end
     end
   end
 end

spec/models/project_services/teamcity_service_spec.rb

@@ -41,13 +41,13 @@ describe TeamcityService, models: true do
           }
         )
       end
-  
+
       it "reset password if url changed" do
         @teamcity_service.teamcity_url = 'http://gitlab1.com'
         @teamcity_service.save
         expect(@teamcity_service.password).to be_nil
       end
-  
+
       it "does not reset password if username changed" do
         @teamcity_service.username = "some_name"
         @teamcity_service.save
@@ -69,7 +69,7 @@ describe TeamcityService, models: true do
         expect(@teamcity_service.password).to be_nil
       end
     end
-    
+
     context "when no password was previously set" do
       before do
         @teamcity_service = TeamcityService.create(
@@ -90,4 +90,134 @@ describe TeamcityService, models: true do
       end
     end
   end
+
+  module TeamcityServiceSpec
+    Response = Struct.new(:code, :data) do
+      def [](key)
+        data[key]
+      end
+    end
+  end
+
+  describe '#build_info' do
+    let(:teamcity_url) { 'http://gitlab.com' }
+    let(:response) { TeamcityServiceSpec::Response.new(200, {}) }
+    subject do
+      TeamcityService.create(
+        project: create(:project),
+        properties: {
+          teamcity_url: teamcity_url,
+          username: 'mic',
+          password: 'password',
+          build_type: 'foo'
+        })
+    end
+    before { allow(HTTParty).to receive(:get).and_return(response) }
+
+    context 'when teamcity_url has no trailing slash' do
+      it { expect(subject.build_info('123')).to eq(response) }
+    end
+
+    context 'when teamcity_url has a trailing slash' do
+      let(:teamcity_url) { 'http://gitlab.com/' }
+
+      it { expect(subject.build_info('123')).to eq(response) }
+    end
+  end
+
+  describe '#build_page' do
+    let(:teamcity_url) { 'http://gitlab.com' }
+    let(:response_code) { 200 }
+    let(:response) do
+      TeamcityServiceSpec::Response.new(response_code, { 'build' => { 'id' => '666' } })
+    end
+    subject do
+      TeamcityService.create(
+        project: create(:project),
+        properties: {
+          teamcity_url: teamcity_url,
+          username: 'mic',
+          password: 'password',
+          build_type: 'foo'
+        })
+    end
+    before { allow(HTTParty).to receive(:get).and_return(response) }
+
+    context 'when teamcity_url has no trailing slash' do
+      it { expect(subject.build_page('123', 'unused')).to eq('http://gitlab.com/viewLog.html?buildId=666&buildTypeId=foo') }
+    end
+
+    context 'when teamcity_url has a trailing slash' do
+      let(:teamcity_url) { 'http://gitlab.com/' }
+
+      it { expect(subject.build_page('123', 'unused')).to eq('http://gitlab.com/viewLog.html?buildId=666&buildTypeId=foo') }
+    end
+
+    context 'when response code is not 200' do
+      let(:response_code) { 500 }
+
+      it { expect(subject.build_page('123', 'unused')).to eq('http://gitlab.com/viewLog.html?buildTypeId=foo') }
+    end
+  end
+
+  describe '#commit_status' do
+    let(:teamcity_url) { 'http://gitlab.com' }
+    let(:response_code) { 200 }
+    let(:build_status) { 'YAY SUCCESS!' }
+    let(:response) do
+      TeamcityServiceSpec::Response.new(response_code, {
+        'build' => {
+          'status' => build_status,
+          'id' => '666'
+        }
+      })
+    end
+    subject do
+      TeamcityService.create(
+        project: create(:project),
+        properties: {
+          teamcity_url: teamcity_url,
+          username: 'mic',
+          password: 'password',
+          build_type: 'foo'
+        })
+    end
+    before { allow(HTTParty).to receive(:get).and_return(response) }
+
+    context 'when response code is not 200' do
+      let(:response_code) { 500 }
+
+      it { expect(subject.commit_status('123', 'unused')).to eq(:error) }
+    end
+
+    context 'when response code is 404' do
+      let(:response_code) { 404 }
+
+      it { expect(subject.commit_status('123', 'unused')).to eq(:pending) }
+    end
+
+    context 'when response code is 200' do
+      context 'when build status contains SUCCESS' do
+        it { expect(subject.commit_status('123', 'unused')).to eq(:success) }
+      end
+
+      context 'when build status contains FAILURE' do
+        let(:build_status) { 'NO FAILURE!' }
+
+        it { expect(subject.commit_status('123', 'unused')).to eq(:failed) }
+      end
+
+      context 'when build status contains Pending' do
+        let(:build_status) { 'NO Pending!' }
+
+        it { expect(subject.commit_status('123', 'unused')).to eq(:pending) }
+      end
+
+      context 'when build status contains anything else' do
+        let(:build_status) { 'FOO BAR!' }
+
+        it { expect(subject.commit_status('123', 'unused')).to eq(:error) }
+      end
+    end
+  end
 end