From 49f72e705fa225175834b5e6b2b1f78f1f608b9c Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Tue, 2 Aug 2016 14:01:22 +0200 Subject: Show deployment status on a MR view --- CHANGELOG | 1 + app/models/deployment.rb | 7 ++++ app/models/environment.rb | 6 ++++ app/models/merge_request.rb | 6 ++++ .../merge_requests/widget/_heading.html.haml | 40 ++++++++++++++-------- 5 files changed, 46 insertions(+), 14 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 8cefaf5d70a..7fb3ccb09ab 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -37,6 +37,7 @@ v 8.11.0 (unreleased) - Update `timeago` plugin to use multiple string/locale settings - Remove unused images (ClemMakesApps) - Limit git rev-list output count to one in forced push check + - Show deployment status on merge requests - Clean up unused routes (Josef Strzibny) - Fix issue on empty project to allow developers to only push to protected branches if given permission - Add green outline to New Branch button. !5447 (winniehell) diff --git a/app/models/deployment.rb b/app/models/deployment.rb index 1a7cd60817e..67a4f3998ec 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -36,4 +36,11 @@ class Deployment < ActiveRecord::Base def manual_actions deployable.try(:other_actions) end + + def deployed_to(ref) + commit = project.commit(ref) + return false unless commit + + project.repository.merge_base(commit.id, sha) == commit.id + end end diff --git a/app/models/environment.rb b/app/models/environment.rb index baed106e8c8..f6fdb8d1ecf 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -25,4 +25,10 @@ class Environment < ActiveRecord::Base def nullify_external_url self.external_url = nil if self.external_url.blank? end + + def deployed_to?(ref) + return unless last_deployment + + last_deployment.deployed_to(ref) + end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index b1fb3ce5d69..85e4d1f6b51 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -590,6 +590,12 @@ class MergeRequest < ActiveRecord::Base !pipeline || pipeline.success? end + def environments + target_project.environments.select do |environment| + environment.deployed_to?(ref_path) + end + end + def state_human_name if merged? "Merged" diff --git a/app/views/projects/merge_requests/widget/_heading.html.haml b/app/views/projects/merge_requests/widget/_heading.html.haml index 6ef640bb654..0581659b742 100644 --- a/app/views/projects/merge_requests/widget/_heading.html.haml +++ b/app/views/projects/merge_requests/widget/_heading.html.haml @@ -1,21 +1,22 @@ - if @pipeline .mr-widget-heading - - %w[success success_with_warnings skipped canceled failed running pending].each do |status| - .ci_widget{ class: "ci-#{status}", style: ("display:none" unless @pipeline.status == status) } - = ci_icon_for_status(status) - %span - CI build - = ci_label_for_status(status) - for - - commit = @merge_request.diff_head_commit - = succeed "." do - = link_to @pipeline.short_sha, namespace_project_commit_path(@merge_request.source_project.namespace, @merge_request.source_project, @pipeline.sha), class: "monospace" - %span.ci-coverage - = link_to "View details", builds_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: "js-show-tab", data: {action: 'builds'} + - @merge_request.environments.each do |environments| + - %w[success success_with_warnings skipped canceled failed running pending].each do |status| + .ci_widget{ class: "ci-#{status}", style: ("display:none" unless @pipeline.status == status) } + = ci_icon_for_status(status) + %span + CI build + = ci_label_for_status(status) + for + - commit = @merge_request.diff_head_commit + = succeed "." do + = link_to @pipeline.short_sha, namespace_project_commit_path(@merge_request.source_project.namespace, @merge_request.source_project, @pipeline.sha), class: "monospace" + %span.ci-coverage + = link_to "View details", builds_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: "js-show-tab", data: {action: 'builds'} - elsif @merge_request.has_ci? - - # Compatibility with old CI integrations (ex jenkins) when you request status from CI server via AJAX - - # Remove in later versions when services like Jenkins will set CI status via Commit status API + // Compatibility with old CI integrations (ex jenkins) when you request status from CI server via AJAX + // Remove in later versions when services like Jenkins will set CI status via Commit status API .mr-widget-heading - %w[success skipped canceled failed running pending].each do |status| .ci_widget{class: "ci-#{status}", style: "display:none"} @@ -42,3 +43,14 @@ .ci_widget.ci-error{style: "display:none"} = icon("times-circle") Could not connect to the CI server. Please check your settings and try again. + +- @merge_request.environments.each do |environment| + .mr-widget-heading + .ci_widget{ class: "ci-success" } + = ci_icon_for_status("success") + %span + Released to #{environment.name} + = succeed '.' do + = time_ago_with_tooltip(environment.created_at, placement: 'bottom') + - if environment.external_url + = link_to icon('external-link'), environment.external_url -- cgit v1.2.1 From 826862d48ef80ddd849b9e3cb05ef37ba7be41e9 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Wed, 3 Aug 2016 10:22:01 +0200 Subject: Tests for release status heading on MR#show --- .../merge_requests/widget/_heading.html.haml | 4 ++-- .../merge_requests/_heading.html.haml_spec.rb | 23 ++++++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) create mode 100644 spec/views/projects/merge_requests/_heading.html.haml_spec.rb diff --git a/app/views/projects/merge_requests/widget/_heading.html.haml b/app/views/projects/merge_requests/widget/_heading.html.haml index 0581659b742..9590c1dbbd1 100644 --- a/app/views/projects/merge_requests/widget/_heading.html.haml +++ b/app/views/projects/merge_requests/widget/_heading.html.haml @@ -15,8 +15,8 @@ = link_to "View details", builds_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: "js-show-tab", data: {action: 'builds'} - elsif @merge_request.has_ci? - // Compatibility with old CI integrations (ex jenkins) when you request status from CI server via AJAX - // Remove in later versions when services like Jenkins will set CI status via Commit status API + - # Compatibility with old CI integrations (ex jenkins) when you request status from CI server via AJAX + - # Remove in later versions when services like Jenkins will set CI status via Commit status API .mr-widget-heading - %w[success skipped canceled failed running pending].each do |status| .ci_widget{class: "ci-#{status}", style: "display:none"} diff --git a/spec/views/projects/merge_requests/_heading.html.haml_spec.rb b/spec/views/projects/merge_requests/_heading.html.haml_spec.rb new file mode 100644 index 00000000000..b78c9c7e9ef --- /dev/null +++ b/spec/views/projects/merge_requests/_heading.html.haml_spec.rb @@ -0,0 +1,23 @@ +require 'spec_helper' + +describe 'projects/merge_requests/widget/_heading' do + include Devise::TestHelpers + + context 'when released to an environment' do + let(:project) { merge_request.target_project } + let(:merge_request) { create(:merge_request, :merged) } + let(:environment) { create(:environment, project: project) } + let!(:deployment) { create(:deployment, environment: environment, + sha: 'a5391128b0ef5d21df5dd23d98557f4ef12fae20') } + + before do + assign(:merge_request, merge_request) + + render + end + + it 'displays that the environment is deployed' do + expect(rendered).to match('Released to') + end + end +end -- cgit v1.2.1 From b497b0ce3fc3c1882639f9c7d55f7991ce41f15d Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Wed, 3 Aug 2016 13:37:39 +0200 Subject: Incorporate feedback --- CHANGELOG | 2 +- app/models/deployment.rb | 4 ++-- app/models/environment.rb | 6 +++--- app/models/merge_request.rb | 2 +- spec/models/deployment_spec.rb | 20 ++++++++++++++++++++ spec/models/environment_spec.rb | 10 ++++++++++ spec/models/merge_request_spec.rb | 15 +++++++++++++++ .../merge_requests/_heading.html.haml_spec.rb | 6 ++++-- 8 files changed, 56 insertions(+), 9 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 7fb3ccb09ab..b26216f33eb 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -37,7 +37,7 @@ v 8.11.0 (unreleased) - Update `timeago` plugin to use multiple string/locale settings - Remove unused images (ClemMakesApps) - Limit git rev-list output count to one in forced push check - - Show deployment status on merge requests + - Show deployment status on merge requests with external URLs - Clean up unused routes (Josef Strzibny) - Fix issue on empty project to allow developers to only push to protected branches if given permission - Add green outline to New Branch button. !5447 (winniehell) diff --git a/app/models/deployment.rb b/app/models/deployment.rb index 67a4f3998ec..19b08f49d96 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -37,10 +37,10 @@ class Deployment < ActiveRecord::Base deployable.try(:other_actions) end - def deployed_to(ref) + def deployed_to?(ref) commit = project.commit(ref) return false unless commit - project.repository.merge_base(commit.id, sha) == commit.id + project.repository.is_ancestor?(commit.id, sha) end end diff --git a/app/models/environment.rb b/app/models/environment.rb index f6fdb8d1ecf..7247125f8a0 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -26,9 +26,9 @@ class Environment < ActiveRecord::Base self.external_url = nil if self.external_url.blank? end - def deployed_to?(ref) - return unless last_deployment + def deployed_from?(ref) + return false unless last_deployment - last_deployment.deployed_to(ref) + last_deployment.deployed_to?(ref) end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 85e4d1f6b51..945b0d76505 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -592,7 +592,7 @@ class MergeRequest < ActiveRecord::Base def environments target_project.environments.select do |environment| - environment.deployed_to?(ref_path) + environment.deployed_from?(ref_path) end end diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb index 7df3df4bb9e..107f8b38acf 100644 --- a/spec/models/deployment_spec.rb +++ b/spec/models/deployment_spec.rb @@ -15,4 +15,24 @@ describe Deployment, models: true do it { is_expected.to validate_presence_of(:ref) } it { is_expected.to validate_presence_of(:sha) } + + describe '#deployed_to?' do + let(:project) { create(:project) } + let(:environment) { create(:environment, project: project) } + let(:deployment) do + create(:deployment, environment: environment, sha: '5f923865dde3436854e9ceb9cdb7815618d4e849') + end + + context 'when there is no project commit' do + it 'returns false' do + expect(deployment.deployed_to?('random-branch')).to be false + end + end + + context 'when they share the same tree branch' do + it 'returns true' do + expect(deployment.deployed_to?('HEAD')).to be true + end + end + end end diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index 8a84ac0a7c7..e65b4f82eff 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -30,4 +30,14 @@ describe Environment, models: true do expect(env.external_url).to be_nil end end + + describe '#deployed_from?' do + let(:environment) { create(:environment) } + + context 'without a last deployment' do + it "returns false" do + expect(environment.deployed_from?('HEAD')).to be false + end + end + end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 3270b877c1a..0727dd29951 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -674,6 +674,21 @@ describe MergeRequest, models: true do end end + describe "#environments" do + let(:project) { create(:project) } + + let!(:deployment) { create(:deployment, environment: environment, sha: '5f923865dde3436854e9ceb9cdb7815618d4e849') } + + let!(:environment) { create(:environment, project: project) } + let!(:environment1) { create(:environment, project: project) } + + let(:merge_request) { create(:merge_request, source_project: project) } + + it 'selects deployed environments' do + expect(merge_request.environments).to eq [environment] + end + end + describe "#reload_diff" do let(:note) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject) } diff --git a/spec/views/projects/merge_requests/_heading.html.haml_spec.rb b/spec/views/projects/merge_requests/_heading.html.haml_spec.rb index b78c9c7e9ef..843a496f4c3 100644 --- a/spec/views/projects/merge_requests/_heading.html.haml_spec.rb +++ b/spec/views/projects/merge_requests/_heading.html.haml_spec.rb @@ -7,8 +7,10 @@ describe 'projects/merge_requests/widget/_heading' do let(:project) { merge_request.target_project } let(:merge_request) { create(:merge_request, :merged) } let(:environment) { create(:environment, project: project) } - let!(:deployment) { create(:deployment, environment: environment, - sha: 'a5391128b0ef5d21df5dd23d98557f4ef12fae20') } + let!(:deployment) do + create(:deployment, environment: environment, + sha: 'a5391128b0ef5d21df5dd23d98557f4ef12fae20') + end before do assign(:merge_request, merge_request) -- cgit v1.2.1 From 03ea01946524a74773b24430c81804c2724b84b6 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Thu, 4 Aug 2016 11:29:41 +0200 Subject: CI build status not per environment --- .../merge_requests/widget/_heading.html.haml | 36 ++++++++++------------ spec/models/deployment_spec.rb | 3 +- spec/models/merge_request_spec.rb | 5 ++- .../merge_requests/_heading.html.haml_spec.rb | 2 +- 4 files changed, 24 insertions(+), 22 deletions(-) diff --git a/app/views/projects/merge_requests/widget/_heading.html.haml b/app/views/projects/merge_requests/widget/_heading.html.haml index 9590c1dbbd1..16e923b831c 100644 --- a/app/views/projects/merge_requests/widget/_heading.html.haml +++ b/app/views/projects/merge_requests/widget/_heading.html.haml @@ -1,18 +1,17 @@ - if @pipeline .mr-widget-heading - - @merge_request.environments.each do |environments| - - %w[success success_with_warnings skipped canceled failed running pending].each do |status| - .ci_widget{ class: "ci-#{status}", style: ("display:none" unless @pipeline.status == status) } - = ci_icon_for_status(status) - %span - CI build - = ci_label_for_status(status) - for - - commit = @merge_request.diff_head_commit - = succeed "." do - = link_to @pipeline.short_sha, namespace_project_commit_path(@merge_request.source_project.namespace, @merge_request.source_project, @pipeline.sha), class: "monospace" - %span.ci-coverage - = link_to "View details", builds_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: "js-show-tab", data: {action: 'builds'} + - %w[success success_with_warnings skipped canceled failed running pending].each do |status| + .ci_widget{ class: "ci-#{status}", style: ("display:none" unless @pipeline.status == status) } + = ci_icon_for_status(status) + %span + CI build + = ci_label_for_status(status) + for + - commit = @merge_request.diff_head_commit + = succeed "." do + = link_to @pipeline.short_sha, namespace_project_commit_path(@merge_request.source_project.namespace, @merge_request.source_project, @pipeline.sha), class: "monospace" + %span.ci-coverage + = link_to "View details", builds_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: "js-show-tab", data: {action: 'builds'} - elsif @merge_request.has_ci? - # Compatibility with old CI integrations (ex jenkins) when you request status from CI server via AJAX @@ -48,9 +47,8 @@ .mr-widget-heading .ci_widget{ class: "ci-success" } = ci_icon_for_status("success") - %span - Released to #{environment.name} - = succeed '.' do - = time_ago_with_tooltip(environment.created_at, placement: 'bottom') - - if environment.external_url - = link_to icon('external-link'), environment.external_url + %span.hidden-sm + Released to #{environment.name}. + - external_url = environment.external_url + - if external_url + = link_to icon('external-link', text: "View on #{external_url.gsub(/\A.*?:\/\//, '')}"), external_url diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb index 107f8b38acf..d7cb142dd32 100644 --- a/spec/models/deployment_spec.rb +++ b/spec/models/deployment_spec.rb @@ -20,7 +20,8 @@ describe Deployment, models: true do let(:project) { create(:project) } let(:environment) { create(:environment, project: project) } let(:deployment) do - create(:deployment, environment: environment, sha: '5f923865dde3436854e9ceb9cdb7815618d4e849') + create(:deployment, environment: environment, + sha: '5f923865dde3436854e9ceb9cdb7815618d4e849') end context 'when there is no project commit' do diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 0727dd29951..e605720a2dd 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -677,7 +677,10 @@ describe MergeRequest, models: true do describe "#environments" do let(:project) { create(:project) } - let!(:deployment) { create(:deployment, environment: environment, sha: '5f923865dde3436854e9ceb9cdb7815618d4e849') } + let!(:deployment) do + create(:deployment, environment: environment, + sha: '5f923865dde3436854e9ceb9cdb7815618d4e849') + end let!(:environment) { create(:environment, project: project) } let!(:environment1) { create(:environment, project: project) } diff --git a/spec/views/projects/merge_requests/_heading.html.haml_spec.rb b/spec/views/projects/merge_requests/_heading.html.haml_spec.rb index 843a496f4c3..0302f14e660 100644 --- a/spec/views/projects/merge_requests/_heading.html.haml_spec.rb +++ b/spec/views/projects/merge_requests/_heading.html.haml_spec.rb @@ -19,7 +19,7 @@ describe 'projects/merge_requests/widget/_heading' do end it 'displays that the environment is deployed' do - expect(rendered).to match('Released to') + expect(rendered).to match("Released to #{environment.name}") end end end -- cgit v1.2.1 From 07fc2f852a0b4136b6d97c1d9773819c47e7e8e7 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Tue, 9 Aug 2016 15:11:14 +0200 Subject: Method names changed to #includes_commit? --- app/assets/stylesheets/pages/merge_requests.scss | 5 +++- app/models/deployment.rb | 3 +-- app/models/environment.rb | 4 +-- app/models/merge_request.rb | 4 ++- .../merge_requests/widget/_heading.html.haml | 9 ++++--- db/schema.rb | 2 +- spec/models/deployment_spec.rb | 13 +++++---- spec/models/environment_spec.rb | 31 +++++++++++++++++++--- spec/models/merge_request_spec.rb | 11 +++----- .../merge_requests/_heading.html.haml_spec.rb | 7 ++--- 10 files changed, 60 insertions(+), 29 deletions(-) diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 0a661e529f0..b4636269518 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -69,6 +69,10 @@ &.ci-success { color: $gl-success; + + a.environment { + color: inherit; + } } &.ci-success_with_warnings { @@ -126,7 +130,6 @@ &.has-conflicts .fa-exclamation-triangle { color: $gl-warning; } - } p:last-child { diff --git a/app/models/deployment.rb b/app/models/deployment.rb index 19b08f49d96..1e338889714 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -37,8 +37,7 @@ class Deployment < ActiveRecord::Base deployable.try(:other_actions) end - def deployed_to?(ref) - commit = project.commit(ref) + def includes_commit?(commit) return false unless commit project.repository.is_ancestor?(commit.id, sha) diff --git a/app/models/environment.rb b/app/models/environment.rb index 7247125f8a0..75e6f869786 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -26,9 +26,9 @@ class Environment < ActiveRecord::Base self.external_url = nil if self.external_url.blank? end - def deployed_from?(ref) + def includes_commit?(commit) return false unless last_deployment - last_deployment.deployed_to?(ref) + last_deployment.includes_commit?(commit) end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 945b0d76505..491ee2792ec 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -591,8 +591,10 @@ class MergeRequest < ActiveRecord::Base end def environments + return unless diff_head_commit + target_project.environments.select do |environment| - environment.deployed_from?(ref_path) + environment.includes_commit?(diff_head_commit) end end diff --git a/app/views/projects/merge_requests/widget/_heading.html.haml b/app/views/projects/merge_requests/widget/_heading.html.haml index 16e923b831c..494695a03a5 100644 --- a/app/views/projects/merge_requests/widget/_heading.html.haml +++ b/app/views/projects/merge_requests/widget/_heading.html.haml @@ -45,10 +45,13 @@ - @merge_request.environments.each do |environment| .mr-widget-heading - .ci_widget{ class: "ci-success" } + .ci_widget.ci-success = ci_icon_for_status("success") %span.hidden-sm - Released to #{environment.name}. + Deployed to + = succeed '.' do + = link_to environment.name, namespace_project_environment_path(@project.namespace, @project, environment), class: 'environment' - external_url = environment.external_url - if external_url - = link_to icon('external-link', text: "View on #{external_url.gsub(/\A.*?:\/\//, '')}"), external_url + = link_to external_url, target: '_blank' do + = icon('external-link', text: "View on #{external_url.gsub(/\A.*?:\/\//, '')}", right: true) diff --git a/db/schema.rb b/db/schema.rb index 6c85e1e9dba..1aa4e8a73d0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -589,12 +589,12 @@ ActiveRecord::Schema.define(version: 20160810142633) do t.datetime "locked_at" t.integer "updated_by_id" t.string "merge_error" - t.text "merge_params" t.boolean "merge_when_build_succeeds", default: false, null: false t.integer "merge_user_id" t.string "merge_commit_sha" t.datetime "deleted_at" t.string "in_progress_merge_commit_sha" + t.text "merge_params" end add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb index d7cb142dd32..bfff639ad78 100644 --- a/spec/models/deployment_spec.rb +++ b/spec/models/deployment_spec.rb @@ -16,23 +16,26 @@ describe Deployment, models: true do it { is_expected.to validate_presence_of(:ref) } it { is_expected.to validate_presence_of(:sha) } - describe '#deployed_to?' do + describe '#includes_commit?' do let(:project) { create(:project) } let(:environment) { create(:environment, project: project) } let(:deployment) do - create(:deployment, environment: environment, - sha: '5f923865dde3436854e9ceb9cdb7815618d4e849') + create(:deployment, environment: environment, sha: project.commit.id) end context 'when there is no project commit' do it 'returns false' do - expect(deployment.deployed_to?('random-branch')).to be false + commit = project.commit('feature') + + expect(deployment.includes_commit?(commit)).to be false end end context 'when they share the same tree branch' do it 'returns true' do - expect(deployment.deployed_to?('HEAD')).to be true + commit = project.commit + + expect(deployment.includes_commit?(commit)).to be true end end end diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index e65b4f82eff..c881897926e 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -31,12 +31,35 @@ describe Environment, models: true do end end - describe '#deployed_from?' do - let(:environment) { create(:environment) } - + describe '#includes_commit?' do context 'without a last deployment' do it "returns false" do - expect(environment.deployed_from?('HEAD')).to be false + expect(environment.includes_commit?('HEAD')).to be false + end + end + + context 'with a last deployment' do + let(:project) { create(:project) } + let(:environment) { create(:environment, project: project) } + + let!(:deployment) do + create(:deployment, environment: environment, sha: project.commit('master').id) + end + + context 'in the same branch' do + it 'returns true' do + expect(environment.includes_commit?(RepoHelpers.sample_commit)).to be true + end + end + + context 'not in the same branch' do + before do + deployment.update(sha: project.commit('feature').id) + end + + it 'returns false' do + expect(environment.includes_commit?(RepoHelpers.sample_commit)).to be false + end end end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index e605720a2dd..35a4418ebb3 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -676,18 +676,15 @@ describe MergeRequest, models: true do describe "#environments" do let(:project) { create(:project) } - - let!(:deployment) do - create(:deployment, environment: environment, - sha: '5f923865dde3436854e9ceb9cdb7815618d4e849') - end - let!(:environment) { create(:environment, project: project) } let!(:environment1) { create(:environment, project: project) } - + let!(:environment2) { create(:environment, project: project) } let(:merge_request) { create(:merge_request, source_project: project) } it 'selects deployed environments' do + create(:deployment, environment: environment, sha: project.commit('master').id) + create(:deployment, environment: environment1, sha: project.commit('feature').id) + expect(merge_request.environments).to eq [environment] end end diff --git a/spec/views/projects/merge_requests/_heading.html.haml_spec.rb b/spec/views/projects/merge_requests/_heading.html.haml_spec.rb index 0302f14e660..733b2dfa7ff 100644 --- a/spec/views/projects/merge_requests/_heading.html.haml_spec.rb +++ b/spec/views/projects/merge_requests/_heading.html.haml_spec.rb @@ -8,18 +8,19 @@ describe 'projects/merge_requests/widget/_heading' do let(:merge_request) { create(:merge_request, :merged) } let(:environment) { create(:environment, project: project) } let!(:deployment) do - create(:deployment, environment: environment, - sha: 'a5391128b0ef5d21df5dd23d98557f4ef12fae20') + create(:deployment, environment: environment, sha: project.commit('master').id) end before do assign(:merge_request, merge_request) + assign(:project, project) render end it 'displays that the environment is deployed' do - expect(rendered).to match("Released to #{environment.name}") + expect(rendered).to match("Deployed to") + expect(rendered).to match("#{environment.name}") end end end -- cgit v1.2.1 From 1f2253545ba7a902212bace29f144a2246eeedab Mon Sep 17 00:00:00 2001 From: Paco Guzman Date: Fri, 8 Jul 2016 18:42:47 +0200 Subject: Use cache for todos counter calling TodoService --- CHANGELOG | 1 + app/models/user.rb | 4 ++-- lib/api/todos.rb | 2 +- spec/requests/api/todos_spec.rb | 12 ++++++++++++ 4 files changed, 16 insertions(+), 3 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 6fe1720796d..c3b4c28dc84 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -270,6 +270,7 @@ v 8.10.0 - Fix new snippet style bug (elliotec) - Instrument Rinku usage - Be explicit to define merge request discussion variables + - Use cache for todos counter calling TodoService - Metrics for Rouge::Plugins::Redcarpet and Rouge::Formatters::HTMLGitlab - RailsCache metris now includes fetch_hit/fetch_miss and read_hit/read_miss info. - Allow [ci skip] to be in any case and allow [skip ci]. !4785 (simon_w) diff --git a/app/models/user.rb b/app/models/user.rb index 73368be7b1b..87a2d999843 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -809,13 +809,13 @@ class User < ActiveRecord::Base def todos_done_count(force: false) Rails.cache.fetch(['users', id, 'todos_done_count'], force: force) do - todos.done.count + TodosFinder.new(self, state: :done).execute.count end end def todos_pending_count(force: false) Rails.cache.fetch(['users', id, 'todos_pending_count'], force: force) do - todos.pending.count + TodosFinder.new(self, state: :pending).execute.count end end diff --git a/lib/api/todos.rb b/lib/api/todos.rb index 26c24c3baff..a90a667fafe 100644 --- a/lib/api/todos.rb +++ b/lib/api/todos.rb @@ -73,7 +73,7 @@ module API # delete do todos = find_todos - todos.each(&:done) + TodoService.new.mark_todos_as_done(todos, current_user) todos.length end diff --git a/spec/requests/api/todos_spec.rb b/spec/requests/api/todos_spec.rb index 3ccd0af652f..887a2ba5b84 100644 --- a/spec/requests/api/todos_spec.rb +++ b/spec/requests/api/todos_spec.rb @@ -117,6 +117,12 @@ describe API::Todos, api: true do expect(response.status).to eq(200) expect(pending_1.reload).to be_done end + + it 'updates todos cache' do + expect_any_instance_of(User).to receive(:update_todos_count_cache).and_call_original + + delete api("/todos/#{pending_1.id}", john_doe) + end end end @@ -139,6 +145,12 @@ describe API::Todos, api: true do expect(pending_2.reload).to be_done expect(pending_3.reload).to be_done end + + it 'updates todos cache' do + expect_any_instance_of(User).to receive(:update_todos_count_cache).and_call_original + + delete api("/todos", john_doe) + end end end -- cgit v1.2.1 From f8b53ba20b74181a46985b0c7dde742239bd54f8 Mon Sep 17 00:00:00 2001 From: Paco Guzman Date: Thu, 11 Aug 2016 18:39:50 +0200 Subject: Recover usage of Todos counter cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’re being kept up to date the counter data but we’re not using it. The only thing which is not real if is the number of projects that the user read changes the number of todos can be stale for some time. The counters will be sync just after the user receives a new todo or mark any as done --- app/controllers/dashboard/todos_controller.rb | 4 +-- app/helpers/todos_helper.rb | 4 +-- app/services/todo_service.rb | 3 ++- lib/api/todos.rb | 6 ++--- spec/services/todo_service_spec.rb | 36 +++++++++++++++++++++++++++ 5 files changed, 44 insertions(+), 9 deletions(-) diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb index 19a76a5b5d8..1243bb96d4d 100644 --- a/app/controllers/dashboard/todos_controller.rb +++ b/app/controllers/dashboard/todos_controller.rb @@ -37,8 +37,8 @@ class Dashboard::TodosController < Dashboard::ApplicationController def todos_counts { - count: TodosFinder.new(current_user, state: :pending).execute.count, - done_count: TodosFinder.new(current_user, state: :done).execute.count + count: current_user.todos_pending_count, + done_count: current_user.todos_done_count } end end diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb index e3a208f826a..0465327060e 100644 --- a/app/helpers/todos_helper.rb +++ b/app/helpers/todos_helper.rb @@ -1,10 +1,10 @@ module TodosHelper def todos_pending_count - @todos_pending_count ||= TodosFinder.new(current_user, state: :pending).execute.count + @todos_pending_count ||= current_user.todos_pending_count end def todos_done_count - @todos_done_count ||= TodosFinder.new(current_user, state: :done).execute.count + @todos_done_count ||= current_user.todos_done_count end def todo_action_name(todo) diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index 6b48d68cccb..eb833dd82ac 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -144,8 +144,9 @@ class TodoService def mark_todos_as_done(todos, current_user) todos = current_user.todos.where(id: todos.map(&:id)) unless todos.respond_to?(:update_all) - todos.update_all(state: :done) + marked_todos = todos.update_all(state: :done) current_user.update_todos_count_cache + marked_todos end # When user marks an issue as todo diff --git a/lib/api/todos.rb b/lib/api/todos.rb index a90a667fafe..19df13d8aac 100644 --- a/lib/api/todos.rb +++ b/lib/api/todos.rb @@ -61,9 +61,9 @@ module API # delete ':id' do todo = current_user.todos.find(params[:id]) - todo.done + TodoService.new.mark_todos_as_done([todo], current_user) - present todo, with: Entities::Todo, current_user: current_user + present todo.reload, with: Entities::Todo, current_user: current_user end # Mark all todos as done @@ -74,8 +74,6 @@ module API delete do todos = find_todos TodoService.new.mark_todos_as_done(todos, current_user) - - todos.length end end end diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index 34d8ea9090e..6c3cbeae13c 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -472,6 +472,42 @@ describe TodoService, services: true do expect(john_doe.todos_pending_count).to eq(1) end + describe '#mark_todos_as_done' do + let(:issue) { create(:issue, project: project, author: author, assignee: john_doe) } + + it 'marks a relation of todos as done' do + create(:todo, :mentioned, user: john_doe, target: issue, project: project) + + todos = TodosFinder.new(john_doe, {}).execute + expect { TodoService.new.mark_todos_as_done(todos, john_doe) } + .to change { john_doe.todos.done.count }.from(0).to(1) + end + + it 'marks an array of todos as done' do + todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project) + + expect { TodoService.new.mark_todos_as_done([todo], john_doe) } + .to change { todo.reload.state }.from('pending').to('done') + end + + it 'returns the number of updated todos' do # Needed on API + todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project) + + expect(TodoService.new.mark_todos_as_done([todo], john_doe)).to eq(1) + end + + it 'caches the number of todos of a user', :caching do + create(:todo, :mentioned, user: john_doe, target: issue, project: project) + todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project) + TodoService.new.mark_todos_as_done([todo], john_doe) + + expect_any_instance_of(TodosFinder).not_to receive(:execute) + + expect(john_doe.todos_done_count).to eq(1) + expect(john_doe.todos_pending_count).to eq(1) + end + end + def should_create_todo(attributes = {}) attributes.reverse_merge!( project: project, -- cgit v1.2.1 From 60858c3d8e5118b5ef6958cf681ba6ae3e141b5c Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Fri, 12 Aug 2016 15:55:53 -0500 Subject: Add archived badge to project listing --- CHANGELOG | 1 + app/views/shared/projects/_project.html.haml | 2 ++ 2 files changed, 3 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index 96965a20f69..6e096b480c0 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -54,6 +54,7 @@ v 8.11.0 (unreleased) - Optimize checking if a user has read access to a list of issues !5370 - Store all DB secrets in secrets.yml, under descriptive names !5274 - Nokogiri's various parsing methods are now instrumented + - Add archived badge to project list !5798 - Add simple identifier to public SSH keys (muteor) - Admin page now references docs instead of a specific file !5600 (AnAverageHuman) - Add a way to send an email and create an issue based on private personal token. Find the email address from issues page. !3363 diff --git a/app/views/shared/projects/_project.html.haml b/app/views/shared/projects/_project.html.haml index 92803838d02..281ec728e41 100644 --- a/app/views/shared/projects/_project.html.haml +++ b/app/views/shared/projects/_project.html.haml @@ -12,6 +12,8 @@ %li.project-row{ class: css_class } = cache(cache_key) do .controls + - if project.archived + %span.label.label-warning archived - if project.commit.try(:status) %span = render_commit_status(project.commit) -- cgit v1.2.1 From 2f06027dc318bd8c4e177444a3168a0129a53687 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Fri, 12 Aug 2016 18:27:42 -0400 Subject: Change the order of the access rules to check simpler first, and add specs --- lib/gitlab/checks/change_access.rb | 2 +- spec/lib/gitlab/checks/change_access_spec.rb | 99 ++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 1 deletion(-) create mode 100644 spec/lib/gitlab/checks/change_access_spec.rb diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb index 52f117e963b..4b32eb966aa 100644 --- a/lib/gitlab/checks/change_access.rb +++ b/lib/gitlab/checks/change_access.rb @@ -11,7 +11,7 @@ module Gitlab end def exec - error = protected_branch_checks || tag_checks || push_checks + error = push_checks || tag_checks || protected_branch_checks if error GitAccessStatus.new(false, error) diff --git a/spec/lib/gitlab/checks/change_access_spec.rb b/spec/lib/gitlab/checks/change_access_spec.rb new file mode 100644 index 00000000000..39069b49978 --- /dev/null +++ b/spec/lib/gitlab/checks/change_access_spec.rb @@ -0,0 +1,99 @@ +require 'spec_helper' + +describe Gitlab::Checks::ChangeAccess, lib: true do + describe '#exec' do + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:user_access) { Gitlab::UserAccess.new(user, project: project) } + let(:changes) do + { + oldrev: 'be93687618e4b132087f430a4d8fc3a609c9b77c', + newrev: '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51', + ref: 'refs/heads/master' + } + end + + subject { described_class.new(changes, project: project, user_access: user_access).exec } + + before { allow(user_access).to receive(:can_do_action?).with(:push_code).and_return(true) } + + context 'without failed checks' do + it "doesn't return any error" do + expect(subject.status).to be(true) + end + end + + context 'when the user is not allowed to push code' do + it 'returns an error' do + expect(user_access).to receive(:can_do_action?).with(:push_code).and_return(false) + + expect(subject.status).to be(false) + expect(subject.message).to eq('You are not allowed to push code to this project.') + end + end + + context 'tags check' do + let(:changes) do + { + oldrev: 'be93687618e4b132087f430a4d8fc3a609c9b77c', + newrev: '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51', + ref: 'refs/tags/v1.0.0' + } + end + + it 'returns an error if the user is not allowed to update tags' do + expect(user_access).to receive(:can_do_action?).with(:admin_project).and_return(false) + + expect(subject.status).to be(false) + expect(subject.message).to eq('You are not allowed to change existing tags on this project.') + end + end + + context 'protected branches check' do + before do + allow(project).to receive(:protected_branch?).with('master').and_return(true) + end + + it 'returns an error if the user is not allowed to do forced pushes to protected branches' do + expect(Gitlab::Checks::ForcePush).to receive(:force_push?).and_return(true) + expect(user_access).to receive(:can_do_action?).with(:force_push_code_to_protected_branches).and_return(false) + + expect(subject.status).to be(false) + expect(subject.message).to eq('You are not allowed to force push code to a protected branch on this project.') + end + + it 'returns an error if the user is not allowed to merge to protected branches' do + expect_any_instance_of(Gitlab::Checks::MatchingMergeRequest).to receive(:match?).and_return(true) + expect(user_access).to receive(:can_merge_to_branch?).and_return(false) + expect(user_access).to receive(:can_push_to_branch?).and_return(false) + + expect(subject.status).to be(false) + expect(subject.message).to eq('You are not allowed to merge code into protected branches on this project.') + end + + it 'returns an error if the user is not allowed to push to protected branches' do + expect(user_access).to receive(:can_push_to_branch?).and_return(false) + + expect(subject.status).to be(false) + expect(subject.message).to eq('You are not allowed to push code to protected branches on this project.') + end + + context 'branch deletion' do + let(:changes) do + { + oldrev: 'be93687618e4b132087f430a4d8fc3a609c9b77c', + newrev: '0000000000000000000000000000000000000000', + ref: 'refs/heads/master' + } + end + + it 'returns an error if the user is not allowed to delete protected branches' do + expect(user_access).to receive(:can_do_action?).with(:remove_protected_branches).and_return(false) + + expect(subject.status).to be(false) + expect(subject.message).to eq('You are not allowed to delete protected branches from this project.') + end + end + end + end +end -- cgit v1.2.1 From 8779da45033d82f8f10d1fa6ca51d59d54d0ed2c Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 11 Aug 2016 19:39:18 -0500 Subject: Don't show new MR URL after push when it doesn't make sense --- app/services/merge_requests/get_urls_service.rb | 13 ++++++++- .../merge_requests/get_urls_service_spec.rb | 34 ++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/app/services/merge_requests/get_urls_service.rb b/app/services/merge_requests/get_urls_service.rb index 501fd135e16..08c1f72d65a 100644 --- a/app/services/merge_requests/get_urls_service.rb +++ b/app/services/merge_requests/get_urls_service.rb @@ -30,10 +30,21 @@ module MergeRequests end def get_branches(changes) + return [] if project.empty_repo? + return [] unless project.merge_requests_enabled + changes_list = Gitlab::ChangesList.new(changes) changes_list.map do |change| next unless Gitlab::Git.branch_ref?(change[:ref]) - Gitlab::Git.branch_name(change[:ref]) + + # Deleted branch + next if Gitlab::Git.blank_ref?(change[:newrev]) + + # Default branch + branch_name = Gitlab::Git.branch_name(change[:ref]) + next if branch_name == project.default_branch + + branch_name end.compact end diff --git a/spec/services/merge_requests/get_urls_service_spec.rb b/spec/services/merge_requests/get_urls_service_spec.rb index ec26770c3eb..8a4b76367e3 100644 --- a/spec/services/merge_requests/get_urls_service_spec.rb +++ b/spec/services/merge_requests/get_urls_service_spec.rb @@ -7,7 +7,9 @@ describe MergeRequests::GetUrlsService do let(:new_merge_request_url) { "http://localhost/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{source_branch}" } let(:show_merge_request_url) { "http://localhost/#{project.namespace.name}/#{project.path}/merge_requests/#{merge_request.iid}" } let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } + let(:deleted_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 #{Gitlab::Git::BLANK_SHA} refs/heads/#{source_branch}" } let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } + let(:default_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/master" } describe "#execute" do shared_examples 'new_merge_request_link' do @@ -32,6 +34,28 @@ describe MergeRequests::GetUrlsService do end end + shared_examples 'no_merge_request_url' do + it 'returns no URL' do + result = service.execute(changes) + expect(result).to be_empty + end + end + + context 'pushing to default branch' do + let(:changes) { default_branch_changes } + it_behaves_like 'no_merge_request_url' + end + + context 'pushing to project with MRs disabled' do + let(:changes) { new_branch_changes } + + before do + project.merge_requests_enabled = false + end + + it_behaves_like 'no_merge_request_url' + end + context 'pushing one completely new branch' do let(:changes) { new_branch_changes } it_behaves_like 'new_merge_request_link' @@ -42,6 +66,11 @@ describe MergeRequests::GetUrlsService do it_behaves_like 'new_merge_request_link' end + context 'pushing to deleted branch' do + let(:changes) { deleted_branch_changes } + it_behaves_like 'no_merge_request_url' + end + context 'pushing to existing branch and merge request opened' do let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch) } let(:changes) { existing_branch_changes } @@ -61,6 +90,11 @@ describe MergeRequests::GetUrlsService do let(:changes) { existing_branch_changes } # Source project is now the forked one let(:service) { MergeRequests::GetUrlsService.new(forked_project) } + + before do + allow(forked_project).to receive(:empty_repo?).and_return(false) + end + it_behaves_like 'show_merge_request_url' end -- cgit v1.2.1 From 60aee5b7afb2ba175606fda59d1f0742f7c4270c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Fri, 12 Aug 2016 21:56:40 -0400 Subject: Add method missing from EE --- app/models/project_wiki.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/models/project_wiki.rb b/app/models/project_wiki.rb index a255710f577..46f70da2452 100644 --- a/app/models/project_wiki.rb +++ b/app/models/project_wiki.rb @@ -56,6 +56,10 @@ class ProjectWiki end end + def repository_exists? + !!repository.exists? + end + def empty? pages.empty? end -- cgit v1.2.1 From d18fe2094b42ccf2ee23b01c4cdcf58dc42aaf03 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Tue, 9 Aug 2016 20:19:14 +0200 Subject: Use new PhantomJS version --- .gitlab-ci.yml | 1 + scripts/prepare_build.sh | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index e8d54e768d3..34ab1f90fca 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -15,6 +15,7 @@ variables: USE_DB: "true" USE_BUNDLE_INSTALL: "true" GIT_DEPTH: "20" + PHANTOMJS_DEB: "phantomjs_2.1.1+dfsg-2_amd64.deb" before_script: - source ./scripts/prepare_build.sh diff --git a/scripts/prepare_build.sh b/scripts/prepare_build.sh index 7e71a030901..9c5a9713189 100755 --- a/scripts/prepare_build.sh +++ b/scripts/prepare_build.sh @@ -20,10 +20,10 @@ if [ -f /.dockerenv ] || [ -f ./dockerinit ]; then # Install phantomjs package pushd vendor/apt - if [ ! -e phantomjs_1.9.8-0jessie_amd64.deb ]; then - wget -q https://gitlab.com/axil/phantomjs-debian/raw/master/phantomjs_1.9.8-0jessie_amd64.deb + if [ ! -e "$PHANTOMJS_DEB" ]; then + wget -q "http://ftp.us.debian.org/debian/pool/main/p/phantomjs/$PHANTOMJS_DEB" fi - dpkg -i phantomjs_1.9.8-0jessie_amd64.deb + dpkg -i "$PHANTOMJS_DEB" popd # Try to install packages -- cgit v1.2.1 From e6173e056756cb65c60c16b02e3a36f84222b6e3 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Tue, 9 Aug 2016 20:25:11 +0200 Subject: Install latest stable phantomjs --- .gitlab-ci.yml | 2 +- scripts/prepare_build.sh | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 34ab1f90fca..be5614520a5 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -15,7 +15,7 @@ variables: USE_DB: "true" USE_BUNDLE_INSTALL: "true" GIT_DEPTH: "20" - PHANTOMJS_DEB: "phantomjs_2.1.1+dfsg-2_amd64.deb" + PHANTOMJS_VERSION: "2.1.1" before_script: - source ./scripts/prepare_build.sh diff --git a/scripts/prepare_build.sh b/scripts/prepare_build.sh index 9c5a9713189..bfca5d76391 100755 --- a/scripts/prepare_build.sh +++ b/scripts/prepare_build.sh @@ -20,10 +20,11 @@ if [ -f /.dockerenv ] || [ -f ./dockerinit ]; then # Install phantomjs package pushd vendor/apt - if [ ! -e "$PHANTOMJS_DEB" ]; then - wget -q "http://ftp.us.debian.org/debian/pool/main/p/phantomjs/$PHANTOMJS_DEB" + PHANTOMJS_FILE="phantomjs-$PHANTOMJS_VERSION-linux-x86_64" + if [ ! -d "$PHANTOMJS_FILE" ]; then + curl -q "https://bitbucket.org/ariya/phantomjs/downloads/$PHANTOMJS_FILE.tar.bz2" | tar jx fi - dpkg -i "$PHANTOMJS_DEB" + cp "$PHANTOMJS_FILE/bin/phantomjs" "/usr/bin/" popd # Try to install packages -- cgit v1.2.1 From 65572b3b9658667962df27376d01d1c55d7d9270 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Tue, 9 Aug 2016 20:29:53 +0200 Subject: Fix file downloading --- scripts/prepare_build.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/prepare_build.sh b/scripts/prepare_build.sh index bfca5d76391..e6a673c94e9 100755 --- a/scripts/prepare_build.sh +++ b/scripts/prepare_build.sh @@ -22,7 +22,7 @@ if [ -f /.dockerenv ] || [ -f ./dockerinit ]; then pushd vendor/apt PHANTOMJS_FILE="phantomjs-$PHANTOMJS_VERSION-linux-x86_64" if [ ! -d "$PHANTOMJS_FILE" ]; then - curl -q "https://bitbucket.org/ariya/phantomjs/downloads/$PHANTOMJS_FILE.tar.bz2" | tar jx + curl -q -L "https://bitbucket.org/ariya/phantomjs/downloads/$PHANTOMJS_FILE.tar.bz2" | tar jx fi cp "$PHANTOMJS_FILE/bin/phantomjs" "/usr/bin/" popd -- cgit v1.2.1 From 2675c9d632c9560a94e98cd3830e2dfdfbbc2bd4 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 11 Aug 2016 11:05:36 +0530 Subject: Fix `U2fSpec` for PhantomJS versions > 2. - We weren't explicilty waiting for the page to load while navigating to the "Manage two-factor authentication" page. This was probably incidentally working for PhantomJS 1.x versions. --- spec/features/u2f_spec.rb | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/spec/features/u2f_spec.rb b/spec/features/u2f_spec.rb index 9335f5bf120..d370f90f7d9 100644 --- a/spec/features/u2f_spec.rb +++ b/spec/features/u2f_spec.rb @@ -1,8 +1,16 @@ require 'spec_helper' feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: true, js: true do + include WaitForAjax + before { allow_any_instance_of(U2fHelper).to receive(:inject_u2f_api?).and_return(true) } + def manage_two_factor_authentication + click_on 'Manage Two-Factor Authentication' + expect(page).to have_content("Setup New U2F Device") + wait_for_ajax + end + def register_u2f_device(u2f_device = nil) u2f_device ||= FakeU2fDevice.new(page) u2f_device.respond_to_u2f_registration @@ -34,7 +42,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: describe 'when 2FA via OTP is enabled' do it 'allows registering a new device' do visit profile_account_path - click_on 'Manage Two-Factor Authentication' + manage_two_factor_authentication expect(page.body).to match("You've already enabled two-factor authentication using mobile") register_u2f_device @@ -46,15 +54,15 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: visit profile_account_path # First device - click_on 'Manage Two-Factor Authentication' + manage_two_factor_authentication register_u2f_device expect(page.body).to match('Your U2F device was registered') # Second device - click_on 'Manage Two-Factor Authentication' + manage_two_factor_authentication register_u2f_device expect(page.body).to match('Your U2F device was registered') - click_on 'Manage Two-Factor Authentication' + manage_two_factor_authentication expect(page.body).to match('You have 2 U2F devices registered') end end @@ -62,7 +70,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: it 'allows the same device to be registered for multiple users' do # First user visit profile_account_path - click_on 'Manage Two-Factor Authentication' + manage_two_factor_authentication u2f_device = register_u2f_device expect(page.body).to match('Your U2F device was registered') logout @@ -71,7 +79,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: user = login_as(:user) user.update_attribute(:otp_required_for_login, true) visit profile_account_path - click_on 'Manage Two-Factor Authentication' + manage_two_factor_authentication register_u2f_device(u2f_device) expect(page.body).to match('Your U2F device was registered') @@ -81,7 +89,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: context "when there are form errors" do it "doesn't register the device if there are errors" do visit profile_account_path - click_on 'Manage Two-Factor Authentication' + manage_two_factor_authentication # Have the "u2f device" respond with bad data page.execute_script("u2f.register = function(_,_,_,callback) { callback('bad response'); };") @@ -96,7 +104,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: it "allows retrying registration" do visit profile_account_path - click_on 'Manage Two-Factor Authentication' + manage_two_factor_authentication # Failed registration page.execute_script("u2f.register = function(_,_,_,callback) { callback('bad response'); };") @@ -122,7 +130,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: login_as(user) user.update_attribute(:otp_required_for_login, true) visit profile_account_path - click_on 'Manage Two-Factor Authentication' + manage_two_factor_authentication @u2f_device = register_u2f_device logout end @@ -161,7 +169,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: current_user = login_as(:user) current_user.update_attribute(:otp_required_for_login, true) visit profile_account_path - click_on 'Manage Two-Factor Authentication' + manage_two_factor_authentication register_u2f_device logout @@ -182,7 +190,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: current_user = login_as(:user) current_user.update_attribute(:otp_required_for_login, true) visit profile_account_path - click_on 'Manage Two-Factor Authentication' + manage_two_factor_authentication register_u2f_device(@u2f_device) logout @@ -248,7 +256,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: user = login_as(:user) user.update_attribute(:otp_required_for_login, true) visit profile_account_path - click_on 'Manage Two-Factor Authentication' + manage_two_factor_authentication expect(page).to have_content("Your U2F device needs to be set up.") register_u2f_device end -- cgit v1.2.1 From f668da11cb225a9b2c2f1d3ba9d1e9fb2797d4f9 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 11 Aug 2016 17:48:32 +0100 Subject: Updated tests --- features/steps/dashboard/event_filters.rb | 6 ++++++ features/steps/dashboard/issues.rb | 5 +++++ features/steps/dashboard/merge_requests.rb | 5 +++++ features/steps/dashboard/new_project.rb | 2 ++ features/steps/project/builds/artifacts.rb | 1 + features/steps/project/forked_merge_requests.rb | 3 +++ features/steps/project/issues/issues.rb | 1 + features/steps/project/merge_requests.rb | 2 ++ features/steps/project/source/browse_files.rb | 1 + features/steps/project/wiki.rb | 2 ++ features/steps/shared/issuable.rb | 4 +--- spec/features/issuables/default_sort_order_spec.rb | 14 +++++++------- spec/features/issues/filter_issues_spec.rb | 2 +- spec/features/merge_requests/create_new_mr_spec.rb | 2 ++ spec/features/profiles/preferences_spec.rb | 4 ++++ .../files/project_owner_creates_license_file_spec.rb | 1 + ...es_link_to_create_license_file_in_empty_project_spec.rb | 1 + spec/features/variables_spec.rb | 1 + 18 files changed, 46 insertions(+), 11 deletions(-) diff --git a/features/steps/dashboard/event_filters.rb b/features/steps/dashboard/event_filters.rb index 726b37cfde5..97b17abb470 100644 --- a/features/steps/dashboard/event_filters.rb +++ b/features/steps/dashboard/event_filters.rb @@ -4,26 +4,32 @@ class Spinach::Features::EventFilters < Spinach::FeatureSteps include SharedProject step 'I should see push event' do + sleep 1 expect(page).to have_selector('span.pushed') end step 'I should not see push event' do + sleep 1 expect(page).not_to have_selector('span.pushed') end step 'I should see new member event' do + sleep 1 expect(page).to have_selector('span.joined') end step 'I should not see new member event' do + sleep 1 expect(page).not_to have_selector('span.joined') end step 'I should see merge request event' do + sleep 1 expect(page).to have_selector('span.accepted') end step 'I should not see merge request event' do + sleep 1 expect(page).not_to have_selector('span.accepted') end diff --git a/features/steps/dashboard/issues.rb b/features/steps/dashboard/issues.rb index 8706f0e8e78..39c65bb6cde 100644 --- a/features/steps/dashboard/issues.rb +++ b/features/steps/dashboard/issues.rb @@ -43,9 +43,14 @@ class Spinach::Features::DashboardIssues < Spinach::FeatureSteps step 'I click "All" link' do find(".js-author-search").click + expect(page).to have_selector(".dropdown-menu-author li a") find(".dropdown-menu-author li a", match: :first).click + expect(page).not_to have_selector(".dropdown-menu-author li a") + find(".js-assignee-search").click + expect(page).to have_selector(".dropdown-menu-assignee li a") find(".dropdown-menu-assignee li a", match: :first).click + expect(page).not_to have_selector(".dropdown-menu-assignee li a") end def should_see(issue) diff --git a/features/steps/dashboard/merge_requests.rb b/features/steps/dashboard/merge_requests.rb index 06db36c7014..6777101fb15 100644 --- a/features/steps/dashboard/merge_requests.rb +++ b/features/steps/dashboard/merge_requests.rb @@ -47,9 +47,14 @@ class Spinach::Features::DashboardMergeRequests < Spinach::FeatureSteps step 'I click "All" link' do find(".js-author-search").click + expect(page).to have_selector(".dropdown-menu-author li a") find(".dropdown-menu-author li a", match: :first).click + expect(page).not_to have_selector(".dropdown-menu-author li a") + find(".js-assignee-search").click + expect(page).to have_selector(".dropdown-menu-assignee li a") find(".dropdown-menu-assignee li a", match: :first).click + expect(page).not_to have_selector(".dropdown-menu-assignee li a") end def should_see(merge_request) diff --git a/features/steps/dashboard/new_project.rb b/features/steps/dashboard/new_project.rb index 727a6a71373..dcfa88f69fc 100644 --- a/features/steps/dashboard/new_project.rb +++ b/features/steps/dashboard/new_project.rb @@ -29,6 +29,7 @@ class Spinach::Features::NewProject < Spinach::FeatureSteps end step 'I am redirected to the GitHub import page' do + expect(page).to have_content('Import Projects from GitHub') expect(current_path).to eq new_import_github_path end @@ -47,6 +48,7 @@ class Spinach::Features::NewProject < Spinach::FeatureSteps end step 'I redirected to Google Code import page' do + expect(page).to have_content('Import projects from Google Code') expect(current_path).to eq new_import_google_code_path end end diff --git a/features/steps/project/builds/artifacts.rb b/features/steps/project/builds/artifacts.rb index b4a32ed2e38..055fca036d3 100644 --- a/features/steps/project/builds/artifacts.rb +++ b/features/steps/project/builds/artifacts.rb @@ -10,6 +10,7 @@ class Spinach::Features::ProjectBuildsArtifacts < Spinach::FeatureSteps step 'I click artifacts browse button' do click_link 'Browse' + expect(page).not_to have_selector('.build-sidebar') end step 'I should see content of artifacts archive' do diff --git a/features/steps/project/forked_merge_requests.rb b/features/steps/project/forked_merge_requests.rb index 6b56a77b832..dacab6c7977 100644 --- a/features/steps/project/forked_merge_requests.rb +++ b/features/steps/project/forked_merge_requests.rb @@ -34,6 +34,9 @@ class Spinach::Features::ProjectForkedMergeRequests < Spinach::FeatureSteps end step 'I fill out a "Merge Request On Forked Project" merge request' do + expect(page).to have_content('Source branch') + expect(page).to have_content('Target branch') + first('.js-source-project').click first('.dropdown-source-project a', text: @forked_project.path_with_namespace) diff --git a/features/steps/project/issues/issues.rb b/features/steps/project/issues/issues.rb index 35f166c7c08..c1592e1d142 100644 --- a/features/steps/project/issues/issues.rb +++ b/features/steps/project/issues/issues.rb @@ -355,5 +355,6 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps def filter_issue(text) fill_in 'issue_search', with: text + find('#issue_search').native.send_keys(:return) end end diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb index a02a54923a5..53d1aedf27f 100644 --- a/features/steps/project/merge_requests.rb +++ b/features/steps/project/merge_requests.rb @@ -489,10 +489,12 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps end step 'I fill in merge request search with "Fe"' do + sleep 1 fill_in 'issue_search', with: "Fe" end step 'I click the "Target branch" dropdown' do + expect(page).to have_content('Target branch') first('.target_branch').click end diff --git a/features/steps/project/source/browse_files.rb b/features/steps/project/source/browse_files.rb index 9a8896acb15..2b74e964ca1 100644 --- a/features/steps/project/source/browse_files.rb +++ b/features/steps/project/source/browse_files.rb @@ -69,6 +69,7 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps end step 'I edit code' do + expect(page).to have_selector('.file-editor') set_new_content end diff --git a/features/steps/project/wiki.rb b/features/steps/project/wiki.rb index 732dc5d0b93..07a955b1a14 100644 --- a/features/steps/project/wiki.rb +++ b/features/steps/project/wiki.rb @@ -142,7 +142,9 @@ class Spinach::Features::ProjectWiki < Spinach::FeatureSteps end step 'I edit the Wiki page with a path' do + expect(page).to have_content('three') click_on 'three' + expect(find('.nav-text')).to have_content('Three') click_on 'Edit' end diff --git a/features/steps/shared/issuable.rb b/features/steps/shared/issuable.rb index b5fd24d246f..aa666a954bc 100644 --- a/features/steps/shared/issuable.rb +++ b/features/steps/shared/issuable.rb @@ -133,9 +133,7 @@ module SharedIssuable end step 'The list should be sorted by "Oldest updated"' do - page.within('.content div.dropdown.inline.prepend-left-10') do - expect(page.find('button.dropdown-toggle.btn')).to have_content('Oldest updated') - end + expect(find('.issues-filters')).to have_content('Oldest updated') end step 'I click link "Next" in the sidebar' do diff --git a/spec/features/issuables/default_sort_order_spec.rb b/spec/features/issuables/default_sort_order_spec.rb index 0d495cd04aa..9114f751b55 100644 --- a/spec/features/issuables/default_sort_order_spec.rb +++ b/spec/features/issuables/default_sort_order_spec.rb @@ -55,7 +55,7 @@ describe 'Projects > Issuables > Default sort order', feature: true do it 'is "last updated"' do visit_merge_requests_with_state(project, 'merged') - expect(selected_sort_order).to eq('last updated') + expect(find('.issues-other-filters')).to have_content('Last updated') expect(first_merge_request).to include(last_updated_issuable.title) expect(last_merge_request).to include(first_updated_issuable.title) end @@ -67,7 +67,7 @@ describe 'Projects > Issuables > Default sort order', feature: true do it 'is "last updated"' do visit_merge_requests_with_state(project, 'closed') - expect(selected_sort_order).to eq('last updated') + expect(find('.issues-other-filters')).to have_content('Last updated') expect(first_merge_request).to include(last_updated_issuable.title) expect(last_merge_request).to include(first_updated_issuable.title) end @@ -79,7 +79,7 @@ describe 'Projects > Issuables > Default sort order', feature: true do it 'is "last created"' do visit_merge_requests_with_state(project, 'all') - expect(selected_sort_order).to eq('last created') + expect(find('.issues-other-filters')).to have_content('Last created') expect(first_merge_request).to include(last_created_issuable.title) expect(last_merge_request).to include(first_created_issuable.title) end @@ -108,7 +108,7 @@ describe 'Projects > Issuables > Default sort order', feature: true do it 'is "last created"' do visit_issues project - expect(selected_sort_order).to eq('last created') + expect(find('.issues-other-filters')).to have_content('Last created') expect(first_issue).to include(last_created_issuable.title) expect(last_issue).to include(first_created_issuable.title) end @@ -120,7 +120,7 @@ describe 'Projects > Issuables > Default sort order', feature: true do it 'is "last created"' do visit_issues_with_state(project, 'open') - expect(selected_sort_order).to eq('last created') + expect(find('.issues-other-filters')).to have_content('Last created') expect(first_issue).to include(last_created_issuable.title) expect(last_issue).to include(first_created_issuable.title) end @@ -132,7 +132,7 @@ describe 'Projects > Issuables > Default sort order', feature: true do it 'is "last updated"' do visit_issues_with_state(project, 'closed') - expect(selected_sort_order).to eq('last updated') + expect(find('.issues-other-filters')).to have_content('Last updated') expect(first_issue).to include(last_updated_issuable.title) expect(last_issue).to include(first_updated_issuable.title) end @@ -144,7 +144,7 @@ describe 'Projects > Issuables > Default sort order', feature: true do it 'is "last created"' do visit_issues_with_state(project, 'all') - expect(selected_sort_order).to eq('last created') + expect(find('.issues-other-filters')).to have_content('Last created') expect(first_issue).to include(last_created_issuable.title) expect(last_issue).to include(first_created_issuable.title) end diff --git a/spec/features/issues/filter_issues_spec.rb b/spec/features/issues/filter_issues_spec.rb index ea81ee54c90..e262f285868 100644 --- a/spec/features/issues/filter_issues_spec.rb +++ b/spec/features/issues/filter_issues_spec.rb @@ -117,7 +117,7 @@ describe 'Filter issues', feature: true do find('.dropdown-menu-user-link', text: user.username).click - wait_for_ajax + expect(page).not_to have_selector('.issues-list .issue') find('.js-label-select').click diff --git a/spec/features/merge_requests/create_new_mr_spec.rb b/spec/features/merge_requests/create_new_mr_spec.rb index e296078bad8..11c9de3c4bf 100644 --- a/spec/features/merge_requests/create_new_mr_spec.rb +++ b/spec/features/merge_requests/create_new_mr_spec.rb @@ -13,6 +13,8 @@ feature 'Create New Merge Request', feature: true, js: true do it 'generates a diff for an orphaned branch' do click_link 'New Merge Request' + expect(page).to have_content('Source branch') + expect(page).to have_content('Target branch') first('.js-source-branch').click first('.dropdown-source-branch .dropdown-content a', text: 'orphaned-branch').click diff --git a/spec/features/profiles/preferences_spec.rb b/spec/features/profiles/preferences_spec.rb index 787bf42d048..259ed7531d8 100644 --- a/spec/features/profiles/preferences_spec.rb +++ b/spec/features/profiles/preferences_spec.rb @@ -68,10 +68,14 @@ describe 'Profile > Preferences', feature: true do allowing_for_delay do find('#logo').click + + expect(page).to have_content('You don\'t have starred projects yet') expect(page.current_path).to eq starred_dashboard_projects_path end click_link 'Your Projects' + + expect(page).not_to have_content('You don\'t have starred projects yet') expect(page.current_path).to eq dashboard_projects_path end end diff --git a/spec/features/projects/files/project_owner_creates_license_file_spec.rb b/spec/features/projects/files/project_owner_creates_license_file_spec.rb index e1e105e6bbe..dbd07464444 100644 --- a/spec/features/projects/files/project_owner_creates_license_file_spec.rb +++ b/spec/features/projects/files/project_owner_creates_license_file_spec.rb @@ -39,6 +39,7 @@ feature 'project owner creates a license file', feature: true, js: true do scenario 'project master creates a license file from the "Add license" link' do click_link 'Add License' + expect(page).to have_content('New File') expect(current_path).to eq( namespace_project_new_blob_path(project.namespace, project, 'master')) expect(find('#file_name').value).to eq('LICENSE') diff --git a/spec/features/projects/files/project_owner_sees_link_to_create_license_file_in_empty_project_spec.rb b/spec/features/projects/files/project_owner_sees_link_to_create_license_file_in_empty_project_spec.rb index 67aac25e427..45bf0c0d038 100644 --- a/spec/features/projects/files/project_owner_sees_link_to_create_license_file_in_empty_project_spec.rb +++ b/spec/features/projects/files/project_owner_sees_link_to_create_license_file_in_empty_project_spec.rb @@ -14,6 +14,7 @@ feature 'project owner sees a link to create a license file in empty project', f visit namespace_project_path(project.namespace, project) click_link 'Create empty bare repository' click_on 'LICENSE' + expect(page).to have_content('New File') expect(current_path).to eq( namespace_project_new_blob_path(project.namespace, project, 'master')) diff --git a/spec/features/variables_spec.rb b/spec/features/variables_spec.rb index 61f2bc61e0c..d7880d5778f 100644 --- a/spec/features/variables_spec.rb +++ b/spec/features/variables_spec.rb @@ -42,6 +42,7 @@ describe 'Project variables', js: true do find('.btn-variable-edit').click end + expect(page).to have_content('Update variable') fill_in('variable_key', with: 'key') fill_in('variable_value', with: 'key value') click_button('Save variable') -- cgit v1.2.1 From 59955fbbbd5f807654e0861f49b182f27654d4cc Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 11 Aug 2016 18:09:17 +0100 Subject: Used mirrored version on GitLab --- scripts/prepare_build.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/prepare_build.sh b/scripts/prepare_build.sh index e6a673c94e9..c1ee808e75d 100755 --- a/scripts/prepare_build.sh +++ b/scripts/prepare_build.sh @@ -22,7 +22,7 @@ if [ -f /.dockerenv ] || [ -f ./dockerinit ]; then pushd vendor/apt PHANTOMJS_FILE="phantomjs-$PHANTOMJS_VERSION-linux-x86_64" if [ ! -d "$PHANTOMJS_FILE" ]; then - curl -q -L "https://bitbucket.org/ariya/phantomjs/downloads/$PHANTOMJS_FILE.tar.bz2" | tar jx + curl -q -L "https://gitlab.com/iamphill/phantomjs/raw/master/$PHANTOMJS_FILE.tar.bz2" | tar jx fi cp "$PHANTOMJS_FILE/bin/phantomjs" "/usr/bin/" popd -- cgit v1.2.1 From 61bdc80dbad253f6532a83ae2554de82deb40e5f Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 12 Aug 2016 10:10:59 +0100 Subject: Updated failing tests --- app/assets/javascripts/issuable.js | 11 ++++------- features/steps/dashboard/dashboard.rb | 1 + features/steps/dashboard/event_filters.rb | 10 ++++++++++ features/steps/project/source/browse_files.rb | 1 + features/support/wait_for_ajax.rb | 11 +++++++++++ 5 files changed, 27 insertions(+), 7 deletions(-) create mode 100644 features/support/wait_for_ajax.rb diff --git a/app/assets/javascripts/issuable.js b/app/assets/javascripts/issuable.js index f27f1bad1f7..d0305c6c6a1 100644 --- a/app/assets/javascripts/issuable.js +++ b/app/assets/javascripts/issuable.js @@ -5,13 +5,10 @@ this.Issuable = { init: function() { - if (!issuable_created) { - issuable_created = true; - Issuable.initTemplates(); - Issuable.initSearch(); - Issuable.initChecks(); - return Issuable.initLabelFilterRemove(); - } + Issuable.initTemplates(); + Issuable.initSearch(); + Issuable.initChecks(); + return Issuable.initLabelFilterRemove(); }, initTemplates: function() { return Issuable.labelRow = _.template('<% _.each(labels, function(label){ %> <%- label.title %> <% }); %>'); diff --git a/features/steps/dashboard/dashboard.rb b/features/steps/dashboard/dashboard.rb index 80ed4c6d64c..a7d61bc28e0 100644 --- a/features/steps/dashboard/dashboard.rb +++ b/features/steps/dashboard/dashboard.rb @@ -26,6 +26,7 @@ class Spinach::Features::Dashboard < Spinach::FeatureSteps end step 'I see prefilled new Merge Request page' do + expect(page).to have_selector('.merge-request-form') expect(current_path).to eq new_namespace_project_merge_request_path(@project.namespace, @project) expect(find("#merge_request_target_project_id").value).to eq @project.id.to_s expect(find("input#merge_request_source_branch").value).to eq "fix" diff --git a/features/steps/dashboard/event_filters.rb b/features/steps/dashboard/event_filters.rb index 97b17abb470..1806bb138eb 100644 --- a/features/steps/dashboard/event_filters.rb +++ b/features/steps/dashboard/event_filters.rb @@ -1,34 +1,44 @@ class Spinach::Features::EventFilters < Spinach::FeatureSteps + include WaitForAjax include SharedAuthentication include SharedPaths include SharedProject step 'I should see push event' do + wait_for_ajax sleep 1 expect(page).to have_selector('span.pushed') end step 'I should not see push event' do + wait_for_ajax + save_and_open_screenshot sleep 1 + save_and_open_screenshot expect(page).not_to have_selector('span.pushed') + save_and_open_screenshot end step 'I should see new member event' do + wait_for_ajax sleep 1 expect(page).to have_selector('span.joined') end step 'I should not see new member event' do + wait_for_ajax sleep 1 expect(page).not_to have_selector('span.joined') end step 'I should see merge request event' do + wait_for_ajax sleep 1 expect(page).to have_selector('span.accepted') end step 'I should not see merge request event' do + wait_for_ajax sleep 1 expect(page).not_to have_selector('span.accepted') end diff --git a/features/steps/project/source/browse_files.rb b/features/steps/project/source/browse_files.rb index 2b74e964ca1..841d191d55b 100644 --- a/features/steps/project/source/browse_files.rb +++ b/features/steps/project/source/browse_files.rb @@ -132,6 +132,7 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps step 'I click on "New file" link in repo' do find('.add-to-tree').click click_link 'New file' + expect(page).to have_selector('.file-editor') end step 'I click on "Upload file" link in repo' do diff --git a/features/support/wait_for_ajax.rb b/features/support/wait_for_ajax.rb new file mode 100644 index 00000000000..b90fc112671 --- /dev/null +++ b/features/support/wait_for_ajax.rb @@ -0,0 +1,11 @@ +module WaitForAjax + def wait_for_ajax + Timeout.timeout(Capybara.default_max_wait_time) do + loop until finished_all_ajax_requests? + end + end + + def finished_all_ajax_requests? + page.evaluate_script('jQuery.active').zero? + end +end -- cgit v1.2.1 From a0cd87d175a2e2c7a19595f7fa4d250309850a35 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 12 Aug 2016 10:41:17 +0100 Subject: Removed screenshot command :poop: --- features/steps/dashboard/event_filters.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/features/steps/dashboard/event_filters.rb b/features/steps/dashboard/event_filters.rb index 1806bb138eb..c9896cbc24a 100644 --- a/features/steps/dashboard/event_filters.rb +++ b/features/steps/dashboard/event_filters.rb @@ -12,11 +12,8 @@ class Spinach::Features::EventFilters < Spinach::FeatureSteps step 'I should not see push event' do wait_for_ajax - save_and_open_screenshot sleep 1 - save_and_open_screenshot expect(page).not_to have_selector('span.pushed') - save_and_open_screenshot end step 'I should see new member event' do -- cgit v1.2.1 From c617cbfcd66a5ded22041e06b5e68454c7145029 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 12 Aug 2016 12:47:17 +0100 Subject: Fixed filtering tests --- features/steps/dashboard/event_filters.rb | 15 +++------------ features/steps/project/issues/issues.rb | 3 ++- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/features/steps/dashboard/event_filters.rb b/features/steps/dashboard/event_filters.rb index c9896cbc24a..2708e191f75 100644 --- a/features/steps/dashboard/event_filters.rb +++ b/features/steps/dashboard/event_filters.rb @@ -5,38 +5,26 @@ class Spinach::Features::EventFilters < Spinach::FeatureSteps include SharedProject step 'I should see push event' do - wait_for_ajax - sleep 1 expect(page).to have_selector('span.pushed') end step 'I should not see push event' do - wait_for_ajax - sleep 1 expect(page).not_to have_selector('span.pushed') end step 'I should see new member event' do - wait_for_ajax - sleep 1 expect(page).to have_selector('span.joined') end step 'I should not see new member event' do - wait_for_ajax - sleep 1 expect(page).not_to have_selector('span.joined') end step 'I should see merge request event' do - wait_for_ajax - sleep 1 expect(page).to have_selector('span.accepted') end step 'I should not see merge request event' do - wait_for_ajax - sleep 1 expect(page).not_to have_selector('span.accepted') end @@ -86,13 +74,16 @@ class Spinach::Features::EventFilters < Spinach::FeatureSteps When 'I click "push" event filter' do click_link("push_event_filter") + sleep 1 end When 'I click "team" event filter' do click_link("team_event_filter") + sleep 1 end When 'I click "merge" event filter' do click_link("merged_event_filter") + sleep 1 end end diff --git a/features/steps/project/issues/issues.rb b/features/steps/project/issues/issues.rb index c1592e1d142..daee90b3767 100644 --- a/features/steps/project/issues/issues.rb +++ b/features/steps/project/issues/issues.rb @@ -354,7 +354,8 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps end def filter_issue(text) + sleep 1 fill_in 'issue_search', with: text - find('#issue_search').native.send_keys(:return) + sleep 1 end end -- cgit v1.2.1 From 5d8ad797503c76ab9818584cf3b2e945ff8cfd68 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 12 Aug 2016 13:39:29 +0100 Subject: Filters test fix --- features/steps/dashboard/event_filters.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/features/steps/dashboard/event_filters.rb b/features/steps/dashboard/event_filters.rb index 2708e191f75..3ff260c0027 100644 --- a/features/steps/dashboard/event_filters.rb +++ b/features/steps/dashboard/event_filters.rb @@ -73,17 +73,20 @@ class Spinach::Features::EventFilters < Spinach::FeatureSteps end When 'I click "push" event filter' do - click_link("push_event_filter") sleep 1 + click_link("Push events") + sleep 2 end When 'I click "team" event filter' do - click_link("team_event_filter") sleep 1 + click_link("Team") + sleep 2 end When 'I click "merge" event filter' do - click_link("merged_event_filter") sleep 1 + click_link("Merge events") + sleep 2 end end -- cgit v1.2.1 From 1392cad8956c490b5ba6106678ab16d790662148 Mon Sep 17 00:00:00 2001 From: Connor Shea Date: Fri, 12 Aug 2016 11:44:43 -0600 Subject: Remove sleeping and replace escaped text. --- features/steps/dashboard/event_filters.rb | 12 ++++++------ spec/features/profiles/preferences_spec.rb | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/features/steps/dashboard/event_filters.rb b/features/steps/dashboard/event_filters.rb index 3ff260c0027..ca3cd0ecc4e 100644 --- a/features/steps/dashboard/event_filters.rb +++ b/features/steps/dashboard/event_filters.rb @@ -73,20 +73,20 @@ class Spinach::Features::EventFilters < Spinach::FeatureSteps end When 'I click "push" event filter' do - sleep 1 + wait_for_ajax click_link("Push events") - sleep 2 + wait_for_ajax end When 'I click "team" event filter' do - sleep 1 + wait_for_ajax click_link("Team") - sleep 2 + wait_for_ajax end When 'I click "merge" event filter' do - sleep 1 + wait_for_ajax click_link("Merge events") - sleep 2 + wait_for_ajax end end diff --git a/spec/features/profiles/preferences_spec.rb b/spec/features/profiles/preferences_spec.rb index 259ed7531d8..d14a1158b67 100644 --- a/spec/features/profiles/preferences_spec.rb +++ b/spec/features/profiles/preferences_spec.rb @@ -69,13 +69,13 @@ describe 'Profile > Preferences', feature: true do allowing_for_delay do find('#logo').click - expect(page).to have_content('You don\'t have starred projects yet') + expect(page).to have_content("You don't have starred projects yet") expect(page.current_path).to eq starred_dashboard_projects_path end click_link 'Your Projects' - expect(page).not_to have_content('You don\'t have starred projects yet') + expect(page).not_to have_content("You don't have starred projects yet") expect(page.current_path).to eq dashboard_projects_path end end -- cgit v1.2.1 From 2cf3c1c31a80d2827c179535362de111ffa72404 Mon Sep 17 00:00:00 2001 From: Alfredo Sumaran Date: Fri, 12 Aug 2016 15:29:39 -0500 Subject: Update phantomjs link --- scripts/prepare_build.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/prepare_build.sh b/scripts/prepare_build.sh index c1ee808e75d..388c1a4dafb 100755 --- a/scripts/prepare_build.sh +++ b/scripts/prepare_build.sh @@ -22,7 +22,7 @@ if [ -f /.dockerenv ] || [ -f ./dockerinit ]; then pushd vendor/apt PHANTOMJS_FILE="phantomjs-$PHANTOMJS_VERSION-linux-x86_64" if [ ! -d "$PHANTOMJS_FILE" ]; then - curl -q -L "https://gitlab.com/iamphill/phantomjs/raw/master/$PHANTOMJS_FILE.tar.bz2" | tar jx + curl -q -L "https://s3.amazonaws.com/gitlab-build-helpers/phantomjs-2.1.1-linux-x86_64.tar.bz2" | tar jx fi cp "$PHANTOMJS_FILE/bin/phantomjs" "/usr/bin/" popd -- cgit v1.2.1 From 504a3b5e6f0b2e2957cf1e4d9d8eebbf32234bdb Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Sun, 14 Aug 2016 22:27:49 +0200 Subject: Fix a memory leak caused by Banzai::Filter::SanitizationFilter In Banzai::Filter::SanitizationFilter#customize_whitelist, we append three lambdas that has reference to the SanitizationFilter instance, which in turn (potentially) has a reference to the following chain: context hash -> Project instance -> Repository instance -> lookup hash -> various Rugged instances -> various mmap-ed git pack files. All of the above is not garbage collected because the array we append the lambdas to is the constant HTML::Pipeline::SanitizationFilter::WHITELIST. --- CHANGELOG | 1 + lib/banzai/filter/sanitization_filter.rb | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index 6e096b480c0..fa9b81d3303 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -108,6 +108,7 @@ v 8.11.0 (unreleased) - Sort folders with submodules in Files view !5521 - Each `File::exists?` replaced to `File::exist?` because of deprecate since ruby version 2.2.0 - Add auto-completition in pipeline (Katarzyna Kobierska Ula Budziszewska) + - Fix a memory leak caused by Banzai::Filter::SanitizationFilter v 8.10.5 - Add a data migration to fix some missing timestamps in the members table. !5670 diff --git a/lib/banzai/filter/sanitization_filter.rb b/lib/banzai/filter/sanitization_filter.rb index ca80aac5a08..6e13282d5f4 100644 --- a/lib/banzai/filter/sanitization_filter.rb +++ b/lib/banzai/filter/sanitization_filter.rb @@ -7,7 +7,7 @@ module Banzai UNSAFE_PROTOCOLS = %w(data javascript vbscript).freeze def whitelist - whitelist = super + whitelist = super.dup customize_whitelist(whitelist) @@ -42,6 +42,8 @@ module Banzai # Allow any protocol in `a` elements... whitelist[:protocols].delete('a') + whitelist[:transformers] = whitelist[:transformers].dup + # ...but then remove links with unsafe protocols whitelist[:transformers].push(remove_unsafe_links) -- cgit v1.2.1 From 4e4ca27ab148f16d9252634e3e2f58447acdeeef Mon Sep 17 00:00:00 2001 From: Egor Lynko Date: Wed, 10 Aug 2016 17:15:27 +0300 Subject: Ability to specify branches for pivotal tracker integration --- CHANGELOG | 1 + .../project_services/pivotaltracker_service.rb | 31 ++++++++-- doc/api/services.md | 4 +- .../pivotaltracker_service_spec.rb | 71 ++++++++++++++++++++++ 4 files changed, 101 insertions(+), 6 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 6e096b480c0..43fe73d9b9a 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.11.0 (unreleased) - Remove the http_parser.rb dependency by removing the tinder gem. !5758 (tbalthazar) + - Ability to specify branches for Pivotal Tracker integration (Egor Lynko) - Fix don't pass a local variable called `i` to a partial. !20510 (herminiotorres) - Fix rename `add_users_into_project` and `projects_ids`. !20512 (herminiotorres) - Fix the title of the toggle dropdown button. !5515 (herminiotorres) diff --git a/app/models/project_services/pivotaltracker_service.rb b/app/models/project_services/pivotaltracker_service.rb index ad19b7795da..5301f9fa0ff 100644 --- a/app/models/project_services/pivotaltracker_service.rb +++ b/app/models/project_services/pivotaltracker_service.rb @@ -1,7 +1,9 @@ class PivotaltrackerService < Service include HTTParty - prop_accessor :token + API_ENDPOINT = 'https://www.pivotaltracker.com/services/v5/source_commits' + + prop_accessor :token, :restrict_to_branch validates :token, presence: true, if: :activated? def title @@ -18,7 +20,17 @@ class PivotaltrackerService < Service def fields [ - { type: 'text', name: 'token', placeholder: '' } + { + type: 'text', + name: 'token', + placeholder: 'Pivotal Tracker API token.' + }, + { + type: 'text', + name: 'restrict_to_branch', + placeholder: 'Comma-separated list of branches which will be ' \ + 'automatically inspected. Leave blank to include all branches.' + } ] end @@ -28,8 +40,8 @@ class PivotaltrackerService < Service def execute(data) return unless supported_events.include?(data[:object_kind]) + return unless allowed_branch?(data[:ref]) - url = 'https://www.pivotaltracker.com/services/v5/source_commits' data[:commits].each do |commit| message = { 'source_commit' => { @@ -40,7 +52,7 @@ class PivotaltrackerService < Service } } PivotaltrackerService.post( - url, + API_ENDPOINT, body: message.to_json, headers: { 'Content-Type' => 'application/json', @@ -49,4 +61,15 @@ class PivotaltrackerService < Service ) end end + + private + + def allowed_branch?(ref) + return true unless ref.present? && restrict_to_branch.present? + + branch = Gitlab::Git.ref_name(ref) + allowed_branches = restrict_to_branch.split(',').map(&:strip) + + branch.present? && allowed_branches.include?(branch) + end end diff --git a/doc/api/services.md b/doc/api/services.md index f821a614047..579fdc0c8c9 100644 --- a/doc/api/services.md +++ b/doc/api/services.md @@ -355,7 +355,7 @@ PUT /projects/:id/services/gemnasium Parameters: -- `api_key` (**required**) - Your personal API KEY on gemnasium.com +- `api_key` (**required**) - Your personal API KEY on gemnasium.com - `token` (**required**) - The project's slug on gemnasium.com ### Delete Gemnasium service @@ -503,6 +503,7 @@ PUT /projects/:id/services/pivotaltracker Parameters: - `token` (**required**) +- `restrict_to_branch` (optional) - Comma-separated list of branches which will be automatically inspected. Leave blank to include all branches. ### Delete PivotalTracker service @@ -661,4 +662,3 @@ Get JetBrains TeamCity CI service settings for a project. ``` GET /projects/:id/services/teamcity ``` - diff --git a/spec/models/project_services/pivotaltracker_service_spec.rb b/spec/models/project_services/pivotaltracker_service_spec.rb index f37edd4d970..d098d988521 100644 --- a/spec/models/project_services/pivotaltracker_service_spec.rb +++ b/spec/models/project_services/pivotaltracker_service_spec.rb @@ -39,4 +39,75 @@ describe PivotaltrackerService, models: true do it { is_expected.not_to validate_presence_of(:token) } end end + + describe 'Execute' do + let(:service) do + PivotaltrackerService.new.tap do |service| + service.token = 'secret_api_token' + end + end + + let(:url) { PivotaltrackerService::API_ENDPOINT } + + def push_data(branch: 'master') + { + object_kind: 'push', + ref: "refs/heads/#{branch}", + commits: [ + { + id: '21c12ea', + author: { + name: 'Some User' + }, + url: 'https://example.com/commit', + message: 'commit message', + } + ] + } + end + + before do + WebMock.stub_request(:post, url) + end + + it 'should post correct message' do + service.execute(push_data) + expect(WebMock).to have_requested(:post, url).with( + body: { + 'source_commit' => { + 'commit_id' => '21c12ea', + 'author' => 'Some User', + 'url' => 'https://example.com/commit', + 'message' => 'commit message' + } + }, + headers: { + 'Content-Type' => 'application/json', + 'X-TrackerToken' => 'secret_api_token' + } + ).once + end + + context 'when allowed branches is specified' do + let(:service) do + super().tap do |service| + service.restrict_to_branch = 'master,v10' + end + end + + it 'should post message if branch is in the list' do + service.execute(push_data(branch: 'master')) + service.execute(push_data(branch: 'v10')) + + expect(WebMock).to have_requested(:post, url).twice + end + + it 'should not post message if branch is not in the list' do + service.execute(push_data(branch: 'mas')) + service.execute(push_data(branch: 'v11')) + + expect(WebMock).not_to have_requested(:post, url) + end + end + end end -- cgit v1.2.1 From 501a7e89978a03e7e10f497c5ad0d4f87319dbe5 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Mon, 15 Aug 2016 11:43:23 +0100 Subject: Used phantomjs variable --- scripts/prepare_build.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/prepare_build.sh b/scripts/prepare_build.sh index 388c1a4dafb..76b2178c79c 100755 --- a/scripts/prepare_build.sh +++ b/scripts/prepare_build.sh @@ -22,7 +22,7 @@ if [ -f /.dockerenv ] || [ -f ./dockerinit ]; then pushd vendor/apt PHANTOMJS_FILE="phantomjs-$PHANTOMJS_VERSION-linux-x86_64" if [ ! -d "$PHANTOMJS_FILE" ]; then - curl -q -L "https://s3.amazonaws.com/gitlab-build-helpers/phantomjs-2.1.1-linux-x86_64.tar.bz2" | tar jx + curl -q -L "https://s3.amazonaws.com/gitlab-build-helpers/$PHANTOMJS_FILE.tar.bz2" | tar jx fi cp "$PHANTOMJS_FILE/bin/phantomjs" "/usr/bin/" popd -- cgit v1.2.1 From d13c36f72de6b1c56e2063dafddd14ecbb430b9a Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Fri, 12 Aug 2016 14:29:59 +0200 Subject: Speed up todos queries by limiting the projects set we join with Closes #20828 --- CHANGELOG | 1 + app/finders/projects_finder.rb | 3 ++- app/finders/todos_finder.rb | 20 ++++++++++---------- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 658bd92a824..c6c2220b133 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -110,6 +110,7 @@ v 8.11.0 (unreleased) - Each `File::exists?` replaced to `File::exist?` because of deprecate since ruby version 2.2.0 - Add auto-completition in pipeline (Katarzyna Kobierska Ula Budziszewska) - Fix a memory leak caused by Banzai::Filter::SanitizationFilter + - Speed up todos queries by limiting the projects set we join with v 8.10.5 - Add a data migration to fix some missing timestamps in the members table. !5670 diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb index 2f0a9659d15..56877b6d75a 100644 --- a/app/finders/projects_finder.rb +++ b/app/finders/projects_finder.rb @@ -1,6 +1,7 @@ class ProjectsFinder < UnionFinder - def execute(current_user = nil, options = {}) + def execute(current_user = nil, options = {}, &block) segments = all_projects(current_user) + segments.map!(&block) if block find_union(segments, Project) end diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index ff866c2faa5..9b24a86e1c1 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -27,9 +27,11 @@ class TodosFinder items = by_action_id(items) items = by_action(items) items = by_author(items) - items = by_project(items) items = by_state(items) items = by_type(items) + # Filtering by project HAS TO be the last because we use + # the project IDs yielded by the todos query thus far + items = by_project(items) items.reorder(id: :desc) end @@ -91,13 +93,10 @@ class TodosFinder @project end - def projects - return @projects if defined?(@projects) - - if project? - @projects = project - else - @projects = ProjectsFinder.new.execute(current_user) + def projects(items) + item_project_ids = items.reorder(nil).select(:project_id) + ProjectsFinder.new.execute(current_user) do |relation| + relation.where(id: item_project_ids) end end @@ -136,8 +135,9 @@ class TodosFinder def by_project(items) if project? items = items.where(project: project) - elsif projects - items = items.merge(projects).joins(:project) + else + item_projects = projects(items) + items = items.merge(item_projects).joins(:project) end items -- cgit v1.2.1 From 76db0dc115316e53ed7d2189cc8789f0a0cef3c2 Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Fri, 12 Aug 2016 15:52:58 +0200 Subject: Pass project IDs relation to ProjectsFinder instead of using a block --- app/finders/projects_finder.rb | 4 ++-- app/finders/todos_finder.rb | 4 +--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb index 56877b6d75a..c7911736812 100644 --- a/app/finders/projects_finder.rb +++ b/app/finders/projects_finder.rb @@ -1,7 +1,7 @@ class ProjectsFinder < UnionFinder - def execute(current_user = nil, options = {}, &block) + def execute(current_user = nil, project_ids_relation = nil) segments = all_projects(current_user) - segments.map!(&block) if block + segments.map! { |s| s.where(id: project_ids_relation) } if project_ids_relation find_union(segments, Project) end diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index 9b24a86e1c1..4fe0070552e 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -95,9 +95,7 @@ class TodosFinder def projects(items) item_project_ids = items.reorder(nil).select(:project_id) - ProjectsFinder.new.execute(current_user) do |relation| - relation.where(id: item_project_ids) - end + ProjectsFinder.new.execute(current_user, item_project_ids) end def type? -- cgit v1.2.1 From e93de6066b8a69bc741fcdf3ef3113dd9b187878 Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Fri, 12 Aug 2016 17:14:39 +0200 Subject: Fix ProjectsFinder spec Follow-up on 1003454c --- spec/finders/projects_finder_spec.rb | 69 +++++++----------------------------- 1 file changed, 12 insertions(+), 57 deletions(-) diff --git a/spec/finders/projects_finder_spec.rb b/spec/finders/projects_finder_spec.rb index 0a1cc3b3df7..e4b4b2d9b20 100644 --- a/spec/finders/projects_finder_spec.rb +++ b/spec/finders/projects_finder_spec.rb @@ -23,71 +23,26 @@ describe ProjectsFinder do let(:finder) { described_class.new } - describe 'without a group' do - describe 'without a user' do - subject { finder.execute } + describe 'without a user' do + subject { finder.execute } - it { is_expected.to eq([public_project]) } - end - - describe 'with a user' do - subject { finder.execute(user) } - - describe 'without private projects' do - it { is_expected.to eq([public_project, internal_project]) } - end - - describe 'with private projects' do - before do - private_project.team.add_user(user, Gitlab::Access::MASTER) - end - - it do - is_expected.to eq([public_project, internal_project, - private_project]) - end - end - end + it { is_expected.to eq([public_project]) } end - describe 'with a group' do - describe 'without a user' do - subject { finder.execute(nil, group: group) } + describe 'with a user' do + subject { finder.execute(user) } - it { is_expected.to eq([public_project]) } + describe 'without private projects' do + it { is_expected.to eq([public_project, internal_project]) } end - describe 'with a user' do - subject { finder.execute(user, group: group) } - - describe 'without shared projects' do - it { is_expected.to eq([public_project, internal_project]) } + describe 'with private projects' do + before do + private_project.team.add_user(user, Gitlab::Access::MASTER) end - describe 'with shared projects and group membership' do - before do - group.add_user(user, Gitlab::Access::DEVELOPER) - - shared_project.project_group_links. - create(group_access: Gitlab::Access::MASTER, group: group) - end - - it do - is_expected.to eq([shared_project, public_project, internal_project]) - end - end - - describe 'with shared projects and project membership' do - before do - shared_project.team.add_user(user, Gitlab::Access::DEVELOPER) - - shared_project.project_group_links. - create(group_access: Gitlab::Access::MASTER, group: group) - end - - it do - is_expected.to eq([shared_project, public_project, internal_project]) - end + it do + is_expected.to eq([public_project, internal_project, private_project]) end end end -- cgit v1.2.1 From be870760d5490a394e3f3e1b7b3fadb8424c38a8 Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Fri, 12 Aug 2016 17:30:44 +0200 Subject: Add a spec for ProjectsFinder project_ids_relation option Follow-up 1003454c --- spec/finders/projects_finder_spec.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/spec/finders/projects_finder_spec.rb b/spec/finders/projects_finder_spec.rb index e4b4b2d9b20..7a3a74335e8 100644 --- a/spec/finders/projects_finder_spec.rb +++ b/spec/finders/projects_finder_spec.rb @@ -46,5 +46,13 @@ describe ProjectsFinder do end end end + + describe 'with project_ids_relation' do + let(:project_ids_relation) { Project.where(id: internal_project.id) } + + subject { finder.execute(user, project_ids_relation) } + + it { is_expected.to eq([internal_project]) } + end end end -- cgit v1.2.1 From 8171544b3d44df6ce810aa436bf87d137bc9b28f Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Fri, 12 Aug 2016 17:19:17 +0200 Subject: Limit the size of SVGs when viewing them as blobs This ensures that SVGs greater than 2 megabytes are not scrubbed and rendered. This in turn prevents requests from timing out due to reading/scrubbing large SVGs potentially taking a lot of time (and memory). The use of 2 megabytes is completely arbitrary. Fixes gitlab-org/gitlab-ce#1435 --- CHANGELOG | 1 + app/models/blob.rb | 7 +++++++ app/views/projects/blob/_image.html.haml | 16 +++++++++++----- spec/models/blob_spec.rb | 22 ++++++++++++++++++++++ 4 files changed, 41 insertions(+), 5 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 6e096b480c0..a1cd42d24f0 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -32,6 +32,7 @@ v 8.11.0 (unreleased) - Add "No one can push" as an option for protected branches. !5081 - Improve performance of AutolinkFilter#text_parse by using XPath - Add experimental Redis Sentinel support !1877 + - Rendering of SVGs as blobs is now limited to SVGs with a size smaller or equal to 2MB - Fix branches page dropdown sort initial state (ClemMakesApps) - Environments have an url to link to - Various redundant database indexes have been removed diff --git a/app/models/blob.rb b/app/models/blob.rb index 0df2805e448..12cc5aaafba 100644 --- a/app/models/blob.rb +++ b/app/models/blob.rb @@ -3,6 +3,9 @@ class Blob < SimpleDelegator CACHE_TIME = 60 # Cache raw blobs referred to by a (mutable) ref for 1 minute CACHE_TIME_IMMUTABLE = 3600 # Cache blobs referred to by an immutable reference for 1 hour + # The maximum size of an SVG that can be displayed. + MAXIMUM_SVG_SIZE = 2.megabytes + # Wrap a Gitlab::Git::Blob object, or return nil when given nil # # This method prevents the decorated object from evaluating to "truthy" when @@ -31,6 +34,10 @@ class Blob < SimpleDelegator text? && language && language.name == 'SVG' end + def size_within_svg_limits? + size <= MAXIMUM_SVG_SIZE + end + def video? UploaderHelper::VIDEO_EXT.include?(extname.downcase.delete('.')) end diff --git a/app/views/projects/blob/_image.html.haml b/app/views/projects/blob/_image.html.haml index 18caddabd39..4c356d1f07f 100644 --- a/app/views/projects/blob/_image.html.haml +++ b/app/views/projects/blob/_image.html.haml @@ -1,9 +1,15 @@ .file-content.image_file - if blob.svg? - - # We need to scrub SVG but we cannot do so in the RawController: it would - - # be wrong/strange if RawController modified the data. - - blob.load_all_data!(@repository) - - blob = sanitize_svg(blob) - %img{src: "data:#{blob.mime_type};base64,#{Base64.encode64(blob.data)}"} + - if blob.size_within_svg_limits? + - # We need to scrub SVG but we cannot do so in the RawController: it would + - # be wrong/strange if RawController modified the data. + - blob.load_all_data!(@repository) + - blob = sanitize_svg(blob) + %img{src: "data:#{blob.mime_type};base64,#{Base64.encode64(blob.data)}"} + - else + .nothing-here-block + The SVG could not be displayed as it is too large, you can + #{link_to('view the raw file', namespace_project_raw_path(@project.namespace, @project, @id), target: '_blank')} + instead. - else %img{src: namespace_project_raw_path(@project.namespace, @project, tree_join(@commit.id, blob.path))} diff --git a/spec/models/blob_spec.rb b/spec/models/blob_spec.rb index 1e5d6a34f83..cee20234e1f 100644 --- a/spec/models/blob_spec.rb +++ b/spec/models/blob_spec.rb @@ -94,4 +94,26 @@ describe Blob do expect(blob.to_partial_path).to eq 'download' end end + + describe '#size_within_svg_limits?' do + let(:blob) { described_class.decorate(double(:blob)) } + + it 'returns true when the blob size is smaller than the SVG limit' do + expect(blob).to receive(:size).and_return(42) + + expect(blob.size_within_svg_limits?).to eq(true) + end + + it 'returns true when the blob size is equal to the SVG limit' do + expect(blob).to receive(:size).and_return(Blob::MAXIMUM_SVG_SIZE) + + expect(blob.size_within_svg_limits?).to eq(true) + end + + it 'returns false when the blob size is larger than the SVG limit' do + expect(blob).to receive(:size).and_return(1.terabyte) + + expect(blob.size_within_svg_limits?).to eq(false) + end + end end -- cgit v1.2.1