From 8b30dd9834fd4026b846b016868701d8e95ec048 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 9 Jan 2017 11:46:15 +0100 Subject: [PATCH 01/40] Extract abstract success with warnings CI/CD status --- lib/gitlab/ci/status/pipeline/factory.rb | 2 +- .../ci/status/pipeline/success_warning.rb | 13 +++++++ .../status/pipeline/success_with_warnings.rb | 31 ---------------- lib/gitlab/ci/status/success_warning.rb | 35 +++++++++++++++++++ .../gitlab/ci/status/pipeline/factory_spec.rb | 2 +- ...rnings_spec.rb => success_warning_spec.rb} | 2 +- 6 files changed, 51 insertions(+), 34 deletions(-) create mode 100644 lib/gitlab/ci/status/pipeline/success_warning.rb delete mode 100644 lib/gitlab/ci/status/pipeline/success_with_warnings.rb create mode 100644 lib/gitlab/ci/status/success_warning.rb rename spec/lib/gitlab/ci/status/pipeline/{success_with_warnings_spec.rb => success_warning_spec.rb} (96%) diff --git a/lib/gitlab/ci/status/pipeline/factory.rb b/lib/gitlab/ci/status/pipeline/factory.rb index 16dcb326be9c46..31e56a485b7223 100644 --- a/lib/gitlab/ci/status/pipeline/factory.rb +++ b/lib/gitlab/ci/status/pipeline/factory.rb @@ -4,7 +4,7 @@ module Status module Pipeline class Factory < Status::Factory def self.extended_statuses - [Pipeline::SuccessWithWarnings] + [Pipeline::SuccessWarning] end def self.common_helpers diff --git a/lib/gitlab/ci/status/pipeline/success_warning.rb b/lib/gitlab/ci/status/pipeline/success_warning.rb new file mode 100644 index 00000000000000..8387682e744490 --- /dev/null +++ b/lib/gitlab/ci/status/pipeline/success_warning.rb @@ -0,0 +1,13 @@ +module Gitlab + module Ci + module Status + module Pipeline + class SuccessWarning < Status::SuccessWarning + def self.matches?(pipeline, user) + pipeline.success? && pipeline.has_warnings? + end + end + end + end + end +end diff --git a/lib/gitlab/ci/status/pipeline/success_with_warnings.rb b/lib/gitlab/ci/status/pipeline/success_with_warnings.rb deleted file mode 100644 index 24bf8b869e04cb..00000000000000 --- a/lib/gitlab/ci/status/pipeline/success_with_warnings.rb +++ /dev/null @@ -1,31 +0,0 @@ -module Gitlab - module Ci - module Status - module Pipeline - class SuccessWithWarnings < SimpleDelegator - include Status::Extended - - def text - 'passed' - end - - def label - 'passed with warnings' - end - - def icon - 'icon_status_warning' - end - - def group - 'success_with_warnings' - end - - def self.matches?(pipeline, user) - pipeline.success? && pipeline.has_warnings? - end - end - end - end - end -end diff --git a/lib/gitlab/ci/status/success_warning.rb b/lib/gitlab/ci/status/success_warning.rb new file mode 100644 index 00000000000000..2affcc08f5043d --- /dev/null +++ b/lib/gitlab/ci/status/success_warning.rb @@ -0,0 +1,35 @@ +module Gitlab + module Ci + module Status + ## + # Abstract extended status used when pipeline/stage/build passed + # conditionally. + # + # This means that failed jobs that are allowed to fail were present. + # + class SuccessWarning < SimpleDelegator + include Status::Extended + + def text + 'passed' + end + + def label + 'passed with warnings' + end + + def icon + 'icon_status_warning' + end + + def group + 'success_with_warnings' + end + + def self.matches?(subject, user) + raise NotImplementedError + end + end + end + end +end diff --git a/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb b/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb index d4a2dc7fcc1e1b..672f5733e98bd2 100644 --- a/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb @@ -49,7 +49,7 @@ it 'fabricates extended "success with warnings" status' do expect(status) - .to be_a Gitlab::Ci::Status::Pipeline::SuccessWithWarnings + .to be_a Gitlab::Ci::Status::Pipeline::SuccessWarning end it 'extends core status with common pipeline methods' do diff --git a/spec/lib/gitlab/ci/status/pipeline/success_with_warnings_spec.rb b/spec/lib/gitlab/ci/status/pipeline/success_warning_spec.rb similarity index 96% rename from spec/lib/gitlab/ci/status/pipeline/success_with_warnings_spec.rb rename to spec/lib/gitlab/ci/status/pipeline/success_warning_spec.rb index 979160eb9c461d..ee3f6174b735a8 100644 --- a/spec/lib/gitlab/ci/status/pipeline/success_with_warnings_spec.rb +++ b/spec/lib/gitlab/ci/status/pipeline/success_warning_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::Ci::Status::Pipeline::SuccessWithWarnings do +describe Gitlab::Ci::Status::Pipeline::SuccessWarning do subject do described_class.new(double('status')) end -- GitLab From 8dbd1e7d0000bb08b6ac6867530bb501eadc85a4 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 9 Jan 2017 12:22:19 +0100 Subject: [PATCH 02/40] Add concrete success warning status to stage factory --- app/models/ci/stage.rb | 8 ++ lib/gitlab/ci/status/pipeline/factory.rb | 2 +- .../ci/status/pipeline/success_warning.rb | 13 ---- lib/gitlab/ci/status/stage/factory.rb | 4 + lib/gitlab/ci/status/success_warning.rb | 6 +- .../gitlab/ci/status/pipeline/factory_spec.rb | 5 +- .../status/pipeline/success_warning_spec.rb | 69 ----------------- .../gitlab/ci/status/stage/factory_spec.rb | 22 ++++++ .../gitlab/ci/status/success_warning_spec.rb | 75 +++++++++++++++++++ 9 files changed, 115 insertions(+), 89 deletions(-) delete mode 100644 lib/gitlab/ci/status/pipeline/success_warning.rb delete mode 100644 spec/lib/gitlab/ci/status/pipeline/success_warning_spec.rb create mode 100644 spec/lib/gitlab/ci/status/success_warning_spec.rb diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index d035eda6df5608..d4b6ff910aa4d4 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -39,5 +39,13 @@ def statuses def builds @builds ||= pipeline.builds.where(stage: name) end + + def success? + status.to_s == 'success' + end + + def has_warnings? + statuses.latest.failed_but_allowed.any? + end end end diff --git a/lib/gitlab/ci/status/pipeline/factory.rb b/lib/gitlab/ci/status/pipeline/factory.rb index 31e56a485b7223..13c8343b12a2b6 100644 --- a/lib/gitlab/ci/status/pipeline/factory.rb +++ b/lib/gitlab/ci/status/pipeline/factory.rb @@ -4,7 +4,7 @@ module Status module Pipeline class Factory < Status::Factory def self.extended_statuses - [Pipeline::SuccessWarning] + [Status::SuccessWarning] end def self.common_helpers diff --git a/lib/gitlab/ci/status/pipeline/success_warning.rb b/lib/gitlab/ci/status/pipeline/success_warning.rb deleted file mode 100644 index 8387682e744490..00000000000000 --- a/lib/gitlab/ci/status/pipeline/success_warning.rb +++ /dev/null @@ -1,13 +0,0 @@ -module Gitlab - module Ci - module Status - module Pipeline - class SuccessWarning < Status::SuccessWarning - def self.matches?(pipeline, user) - pipeline.success? && pipeline.has_warnings? - end - end - end - end - end -end diff --git a/lib/gitlab/ci/status/stage/factory.rb b/lib/gitlab/ci/status/stage/factory.rb index 689a5dd45bc391..4c37f084d07d2c 100644 --- a/lib/gitlab/ci/status/stage/factory.rb +++ b/lib/gitlab/ci/status/stage/factory.rb @@ -3,6 +3,10 @@ module Ci module Status module Stage class Factory < Status::Factory + def self.extended_statuses + [Status::SuccessWarning] + end + def self.common_helpers Status::Stage::Common end diff --git a/lib/gitlab/ci/status/success_warning.rb b/lib/gitlab/ci/status/success_warning.rb index 2affcc08f5043d..d4cdab6957a00c 100644 --- a/lib/gitlab/ci/status/success_warning.rb +++ b/lib/gitlab/ci/status/success_warning.rb @@ -2,9 +2,7 @@ module Gitlab module Ci module Status ## - # Abstract extended status used when pipeline/stage/build passed - # conditionally. - # + # Extended status used when pipeline or stage passed conditionally. # This means that failed jobs that are allowed to fail were present. # class SuccessWarning < SimpleDelegator @@ -27,7 +25,7 @@ def group end def self.matches?(subject, user) - raise NotImplementedError + subject.success? && subject.has_warnings? end end end diff --git a/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb b/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb index 672f5733e98bd2..0df6e881877545 100644 --- a/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb @@ -49,11 +49,12 @@ it 'fabricates extended "success with warnings" status' do expect(status) - .to be_a Gitlab::Ci::Status::Pipeline::SuccessWarning + .to be_a Gitlab::Ci::Status::SuccessWarning end - it 'extends core status with common pipeline methods' do + it 'extends core status with common pipeline method' do expect(status).to have_details + expect(status.details_path).to include "pipelines/#{pipeline.id}" end end end diff --git a/spec/lib/gitlab/ci/status/pipeline/success_warning_spec.rb b/spec/lib/gitlab/ci/status/pipeline/success_warning_spec.rb deleted file mode 100644 index ee3f6174b735a8..00000000000000 --- a/spec/lib/gitlab/ci/status/pipeline/success_warning_spec.rb +++ /dev/null @@ -1,69 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Ci::Status::Pipeline::SuccessWarning do - subject do - described_class.new(double('status')) - end - - describe '#test' do - it { expect(subject.text).to eq 'passed' } - end - - describe '#label' do - it { expect(subject.label).to eq 'passed with warnings' } - end - - describe '#icon' do - it { expect(subject.icon).to eq 'icon_status_warning' } - end - - describe '#group' do - it { expect(subject.group).to eq 'success_with_warnings' } - end - - describe '.matches?' do - context 'when pipeline is successful' do - let(:pipeline) do - create(:ci_pipeline, status: :success) - end - - context 'when pipeline has warnings' do - before do - allow(pipeline).to receive(:has_warnings?).and_return(true) - end - - it 'is a correct match' do - expect(described_class.matches?(pipeline, double)).to eq true - end - end - - context 'when pipeline does not have warnings' do - it 'does not match' do - expect(described_class.matches?(pipeline, double)).to eq false - end - end - end - - context 'when pipeline is not successful' do - let(:pipeline) do - create(:ci_pipeline, status: :skipped) - end - - context 'when pipeline has warnings' do - before do - allow(pipeline).to receive(:has_warnings?).and_return(true) - end - - it 'does not match' do - expect(described_class.matches?(pipeline, double)).to eq false - end - end - - context 'when pipeline does not have warnings' do - it 'does not match' do - expect(described_class.matches?(pipeline, double)).to eq false - end - end - end - end -end diff --git a/spec/lib/gitlab/ci/status/stage/factory_spec.rb b/spec/lib/gitlab/ci/status/stage/factory_spec.rb index 6f8721d30c22c1..a60f84be9e97c4 100644 --- a/spec/lib/gitlab/ci/status/stage/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/stage/factory_spec.rb @@ -42,5 +42,27 @@ end end end + + end + + context 'when stage has warnings' do + let(:stage) do + build(:ci_stage, name: 'test', status: :success, pipeline: pipeline) + end + + before do + create(:ci_build, :allowed_to_fail, :failed, + stage: 'test', pipeline: stage.pipeline) + end + + it 'fabricates extended "success with warnings" status' do + expect(status) + .to be_a Gitlab::Ci::Status::SuccessWarning + end + + it 'extends core status with common stage method' do + expect(status).to have_details + expect(status.details_path).to include "pipelines/#{pipeline.id}##{stage.name}" + end end end diff --git a/spec/lib/gitlab/ci/status/success_warning_spec.rb b/spec/lib/gitlab/ci/status/success_warning_spec.rb new file mode 100644 index 00000000000000..7e2269397c6676 --- /dev/null +++ b/spec/lib/gitlab/ci/status/success_warning_spec.rb @@ -0,0 +1,75 @@ +require 'spec_helper' + +describe Gitlab::Ci::Status::SuccessWarning do + subject do + described_class.new(double('status')) + end + + describe '#test' do + it { expect(subject.text).to eq 'passed' } + end + + describe '#label' do + it { expect(subject.label).to eq 'passed with warnings' } + end + + describe '#icon' do + it { expect(subject.icon).to eq 'icon_status_warning' } + end + + describe '#group' do + it { expect(subject.group).to eq 'success_with_warnings' } + end + + describe '.matches?' do + let(:matchable) { double('matchable') } + + context 'when matchable subject is successful' do + before do + allow(matchable).to receive(:success?).and_return(true) + end + + context 'when matchable subject has warnings' do + before do + allow(matchable).to receive(:has_warnings?).and_return(true) + end + + it 'is a correct match' do + expect(described_class.matches?(matchable, double)).to eq true + end + end + + context 'when matchable subject does not have warnings' do + before do + allow(matchable).to receive(:has_warnings?).and_return(false) + end + + it 'does not match' do + expect(described_class.matches?(matchable, double)).to eq false + end + end + end + + context 'when matchable subject is not successful' do + before do + allow(matchable).to receive(:success?).and_return(false) + end + + context 'when matchable subject has warnings' do + before do + allow(matchable).to receive(:has_warnings?).and_return(true) + end + + it 'does not match' do + expect(described_class.matches?(matchable, double)).to eq false + end + end + + context 'when matchable subject does not have warnings' do + it 'does not match' do + expect(described_class.matches?(matchable, double)).to eq false + end + end + end + end +end -- GitLab From fd115bc130a8bf77814262b67eb0f20f1f19cb85 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 9 Jan 2017 13:07:52 +0100 Subject: [PATCH 03/40] Add specs for two new methods defined in stage class --- .../gitlab/ci/status/stage/factory_spec.rb | 1 - spec/models/ci/stage_spec.rb | 48 +++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/spec/lib/gitlab/ci/status/stage/factory_spec.rb b/spec/lib/gitlab/ci/status/stage/factory_spec.rb index a60f84be9e97c4..bbb40e2c1abbad 100644 --- a/spec/lib/gitlab/ci/status/stage/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/stage/factory_spec.rb @@ -42,7 +42,6 @@ end end end - end context 'when stage has warnings' do diff --git a/spec/models/ci/stage_spec.rb b/spec/models/ci/stage_spec.rb index 742bedb37e4962..3d387d52c8e720 100644 --- a/spec/models/ci/stage_spec.rb +++ b/spec/models/ci/stage_spec.rb @@ -142,6 +142,54 @@ end end + describe '#success?' do + context 'when stage is successful' do + before do + create_job(:ci_build, status: :success) + create_job(:generic_commit_status, status: :success) + end + + it 'is successful' do + expect(stage).to be_success + end + end + + context 'when stage is not successful' do + before do + create_job(:ci_build, status: :failed) + create_job(:generic_commit_status, status: :success) + end + + it 'is not successful' do + expect(stage).not_to be_success + end + end + end + + describe '#has_warnings?' do + context 'when stage has warnings' do + before do + create(:ci_build, :failed, :allowed_to_fail, + stage: stage_name, pipeline: pipeline) + end + + it 'has warnings' do + expect(stage).to have_warnings + end + end + + context 'when stage does not have warnings' do + before do + create(:ci_build, :success, stage: stage_name, + pipeline: pipeline) + end + + it 'does not have warnings' do + expect(stage).not_to have_warnings + end + end + end + def create_job(type, status: 'success', stage: stage_name) create(type, pipeline: pipeline, stage: stage, status: status) end -- GitLab From e5d53df70e6bebf267e5709790842cb674ee96b0 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 9 Jan 2017 14:13:16 +0100 Subject: [PATCH 04/40] Add changelog for warning icon for stage in graph --- .../feature-success-warning-icons-in-stages-builds.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/feature-success-warning-icons-in-stages-builds.yml diff --git a/changelogs/unreleased/feature-success-warning-icons-in-stages-builds.yml b/changelogs/unreleased/feature-success-warning-icons-in-stages-builds.yml new file mode 100644 index 00000000000000..5fba0332881958 --- /dev/null +++ b/changelogs/unreleased/feature-success-warning-icons-in-stages-builds.yml @@ -0,0 +1,4 @@ +--- +title: Use warning icon in mini-graph if stage passed conditionally +merge_request: 8503 +author: -- GitLab From 9f1279184b85927a12b93df00896c57b12373a9a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 11 Jan 2017 13:26:56 +0100 Subject: [PATCH 05/40] Add extended status for build failed but allowed to --- lib/gitlab/ci/status/build/failed_allowed.rb | 27 +++++ .../ci/status/build/failed_allowed_spec.rb | 110 ++++++++++++++++++ 2 files changed, 137 insertions(+) create mode 100644 lib/gitlab/ci/status/build/failed_allowed.rb create mode 100644 spec/lib/gitlab/ci/status/build/failed_allowed_spec.rb diff --git a/lib/gitlab/ci/status/build/failed_allowed.rb b/lib/gitlab/ci/status/build/failed_allowed.rb new file mode 100644 index 00000000000000..807afe24bd5765 --- /dev/null +++ b/lib/gitlab/ci/status/build/failed_allowed.rb @@ -0,0 +1,27 @@ +module Gitlab + module Ci + module Status + module Build + class FailedAllowed < SimpleDelegator + include Status::Extended + + def label + 'failed (allowed to fail)' + end + + def icon + 'icon_status_warning' + end + + def group + 'failed_with_warnings' + end + + def self.matches?(build, user) + build.failed? && build.allow_failure? + end + end + end + end + end +end diff --git a/spec/lib/gitlab/ci/status/build/failed_allowed_spec.rb b/spec/lib/gitlab/ci/status/build/failed_allowed_spec.rb new file mode 100644 index 00000000000000..20f714597385bf --- /dev/null +++ b/spec/lib/gitlab/ci/status/build/failed_allowed_spec.rb @@ -0,0 +1,110 @@ +require 'spec_helper' + +describe Gitlab::Ci::Status::Build::FailedAllowed do + let(:status) { double('core status') } + let(:user) { double('user') } + + subject do + described_class.new(status) + end + + describe '#text' do + it 'does not override status text' do + expect(status).to receive(:text) + + subject.text + end + end + + describe '#icon' do + it 'returns a warning icon' do + expect(subject.icon).to eq 'icon_status_warning' + end + end + + describe '#label' do + it 'returns information about failed but allowed to fail status' do + expect(subject.label).to eq 'failed (allowed to fail)' + end + end + + describe '#group' do + it 'returns status failed with warnings status group' do + expect(subject.group).to eq 'failed_with_warnings' + end + end + + describe 'action details' do + describe '#has_action?' do + it 'does not decorate action details' do + expect(status).to receive(:has_action?) + + subject.has_action? + end + end + + describe '#action_path' do + it 'does not decorate action path' do + expect(status).to receive(:action_path) + + subject.action_path + end + end + + describe '#action_icon' do + it 'does not decorate action icon' do + expect(status).to receive(:action_icon) + + subject.action_icon + end + end + + describe '#action_title' do + it 'does not decorate action title' do + expect(status).to receive(:action_title) + + subject.action_title + end + end + end + + describe '.matches?' do + subject { described_class.matches?(build, user) } + + context 'when build is failed' do + context 'when build is allowed to fail' do + let(:build) { create(:ci_build, :failed, :allowed_to_fail) } + + it 'is a correct match' do + expect(subject).to be true + end + end + + context 'when build is not allowed to fail' do + let(:build) { create(:ci_build, :failed) } + + it 'is not a correct match' do + expect(subject).not_to be true + end + end + end + + context 'when build did not fail' do + context 'when build is allowed to fail' do + let(:build) { create(:ci_build, :success, :allowed_to_fail) } + + it 'is not a correct match' do + expect(subject).not_to be true + end + end + + context 'when build is not allowed to fail' do + let(:build) { create(:ci_build, :success) } + + it 'is not a correct match' do + expect(subject).not_to be true + end + end + end + end +end -- GitLab From 1d01ffb782a1bf44d5826666590408b24fba2332 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 12 Jan 2017 12:45:00 +0100 Subject: [PATCH 06/40] Make it possible to combine extended CI/CD statuses This commit also makes it possible to configure exclusive groups. There can be only one detailed status matched within an exclusive group, which is important from the performance perspective. --- lib/gitlab/ci/status/factory.rb | 18 +++-- spec/lib/gitlab/ci/status/factory_spec.rb | 97 +++++++++++++++++++++-- 2 files changed, 102 insertions(+), 13 deletions(-) diff --git a/lib/gitlab/ci/status/factory.rb b/lib/gitlab/ci/status/factory.rb index ae9ef895df49d2..71c54aebcc359c 100644 --- a/lib/gitlab/ci/status/factory.rb +++ b/lib/gitlab/ci/status/factory.rb @@ -8,10 +8,12 @@ def initialize(subject, user) end def fabricate! - if extended_status - extended_status.new(core_status) - else + if extended_statuses.none? core_status + else + extended_statuses.inject(core_status) do |status, extended| + extended.new(status) + end end end @@ -36,10 +38,14 @@ def core_status .extend(self.class.common_helpers) end - def extended_status - @extended ||= self.class.extended_statuses.find do |status| - status.matches?(@subject, @user) + def extended_statuses + return @extended_statuses if defined?(@extended_statuses) + + groups = self.class.extended_statuses.map do |group| + Array(group).find { |status| status.matches?(@subject, @user) } end + + @extended_statuses = groups.flatten.compact end end end diff --git a/spec/lib/gitlab/ci/status/factory_spec.rb b/spec/lib/gitlab/ci/status/factory_spec.rb index f92a1c149bf1c6..d78d563a9b9f5a 100644 --- a/spec/lib/gitlab/ci/status/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/factory_spec.rb @@ -1,18 +1,14 @@ require 'spec_helper' describe Gitlab::Ci::Status::Factory do - subject do - described_class.new(resource, user) - end - let(:user) { create(:user) } - - let(:status) { subject.fabricate! } + let(:status) { factory.fabricate! } + let(:factory) { described_class.new(resource, user) } context 'when object has a core status' do HasStatus::AVAILABLE_STATUSES.each do |core_status| context "when core status is #{core_status}" do - let(:resource) { double(status: core_status) } + let(:resource) { double('resource', status: core_status) } it "fabricates a core status #{core_status}" do expect(status).to be_a( @@ -21,4 +17,91 @@ end end end + + context 'when resource supports multiple extended statuses' do + let(:resource) { double('resource', status: :success) } + + let(:first_extended_status) do + Class.new(SimpleDelegator) do + def first_method + 'first return value' + end + + def second_method + 'second return value' + end + + def self.matches?(*) + true + end + end + end + + let(:second_extended_status) do + Class.new(SimpleDelegator) do + def first_method + 'decorated return value' + end + + def third_method + 'third return value' + end + + def self.matches?(*) + true + end + end + end + + shared_examples 'compound decorator factory' do + it 'fabricates compound decorator' do + expect(status.first_method).to eq 'decorated return value' + expect(status.second_method).to eq 'second return value' + expect(status.third_method).to eq 'third return value' + end + + it 'delegates to core status' do + expect(status.text).to eq 'passed' + end + + it 'latest matches status becomes a status name' do + expect(status.class).to eq second_extended_status + end + end + + context 'when exclusive statuses are matches' do + before do + allow(described_class).to receive(:extended_statuses) + .and_return([[first_extended_status, second_extended_status]]) + end + + it 'fabricates compound decorator' do + expect(status.first_method).to eq 'first return value' + expect(status.second_method).to eq 'second return value' + expect(status).not_to respond_to(:third_method) + end + + it 'delegates to core status' do + expect(status.text).to eq 'passed' + end + end + + context 'when exclusive statuses are not matched' do + before do + allow(described_class).to receive(:extended_statuses) + .and_return([[first_extended_status], [second_extended_status]]) + end + + it_behaves_like 'compound decorator factory' + end + + context 'when using simplified status grouping' do + before do + allow(described_class).to receive(:extended_statuses) + .and_return([first_extended_status, second_extended_status]) + end + + it_behaves_like 'compound decorator factory' + end + end end -- GitLab From 48c19e7395e501c8e9eaa3975b2510b37f56d46c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 12 Jan 2017 13:04:51 +0100 Subject: [PATCH 07/40] Expose methods that match statuses in status factories --- lib/gitlab/ci/status/factory.rb | 25 ++++----- spec/lib/gitlab/ci/status/factory_spec.rb | 62 ++++++++++++++++------- 2 files changed, 55 insertions(+), 32 deletions(-) diff --git a/lib/gitlab/ci/status/factory.rb b/lib/gitlab/ci/status/factory.rb index 71c54aebcc359c..3da5ae4fc01983 100644 --- a/lib/gitlab/ci/status/factory.rb +++ b/lib/gitlab/ci/status/factory.rb @@ -5,6 +5,7 @@ class Factory def initialize(subject, user) @subject = subject @user = user + @status = subject.status || :created end def fabricate! @@ -17,23 +18,9 @@ def fabricate! end end - def self.extended_statuses - [] - end - - def self.common_helpers - Module.new - end - - private - - def simple_status - @simple_status ||= @subject.status || :created - end - def core_status Gitlab::Ci::Status - .const_get(simple_status.capitalize) + .const_get(@status.capitalize) .new(@subject, @user) .extend(self.class.common_helpers) end @@ -47,6 +34,14 @@ def extended_statuses @extended_statuses = groups.flatten.compact end + + def self.extended_statuses + [] + end + + def self.common_helpers + Module.new + end end end end diff --git a/spec/lib/gitlab/ci/status/factory_spec.rb b/spec/lib/gitlab/ci/status/factory_spec.rb index d78d563a9b9f5a..f1b758640a77a5 100644 --- a/spec/lib/gitlab/ci/status/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/factory_spec.rb @@ -2,17 +2,28 @@ describe Gitlab::Ci::Status::Factory do let(:user) { create(:user) } - let(:status) { factory.fabricate! } + let(:fabricated_status) { factory.fabricate! } let(:factory) { described_class.new(resource, user) } context 'when object has a core status' do - HasStatus::AVAILABLE_STATUSES.each do |core_status| - context "when core status is #{core_status}" do - let(:resource) { double('resource', status: core_status) } + HasStatus::AVAILABLE_STATUSES.each do |simple_status| + context "when simple core status is #{simple_status}" do + let(:resource) { double('resource', status: simple_status) } - it "fabricates a core status #{core_status}" do - expect(status).to be_a( - Gitlab::Ci::Status.const_get(core_status.capitalize)) + let(:expected_status) do + Gitlab::Ci::Status.const_get(simple_status.capitalize) + end + + it "fabricates a core status #{simple_status}" do + expect(fabricated_status).to be_a expected_status + end + + it "matches a valid core status for #{simple_status}" do + expect(factory.core_status).to be_a expected_status + end + + it "does not match any extended statuses for #{simple_status}" do + expect(factory.extended_statuses).to be_empty end end end @@ -55,17 +66,26 @@ def self.matches?(*) shared_examples 'compound decorator factory' do it 'fabricates compound decorator' do - expect(status.first_method).to eq 'decorated return value' - expect(status.second_method).to eq 'second return value' - expect(status.third_method).to eq 'third return value' + expect(fabricated_status.first_method).to eq 'decorated return value' + expect(fabricated_status.second_method).to eq 'second return value' + expect(fabricated_status.third_method).to eq 'third return value' end it 'delegates to core status' do - expect(status.text).to eq 'passed' + expect(fabricated_status.text).to eq 'passed' end it 'latest matches status becomes a status name' do - expect(status.class).to eq second_extended_status + expect(fabricated_status.class).to eq second_extended_status + end + + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Success + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [first_extended_status, second_extended_status] end end @@ -75,14 +95,22 @@ def self.matches?(*) .and_return([[first_extended_status, second_extended_status]]) end - it 'fabricates compound decorator' do - expect(status.first_method).to eq 'first return value' - expect(status.second_method).to eq 'second return value' - expect(status).not_to respond_to(:third_method) + it 'does not fabricate compound decorator' do + expect(fabricated_status.first_method).to eq 'first return value' + expect(fabricated_status.second_method).to eq 'second return value' + expect(fabricated_status).not_to respond_to(:third_method) end it 'delegates to core status' do - expect(status.text).to eq 'passed' + expect(fabricated_status.text).to eq 'passed' + end + + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Success + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses).to eq [first_extended_status] end end -- GitLab From 6053049f4a8822bf39bbe8e1fa34c5e3522b63e9 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 12 Jan 2017 13:22:04 +0100 Subject: [PATCH 08/40] Add new CI/CD status failed with warnings icon group --- app/assets/stylesheets/framework/icons.scss | 1 + app/assets/stylesheets/pages/status.scss | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/framework/icons.scss b/app/assets/stylesheets/framework/icons.scss index dccf5177e35133..868f28cd3564f6 100644 --- a/app/assets/stylesheets/framework/icons.scss +++ b/app/assets/stylesheets/framework/icons.scss @@ -15,6 +15,7 @@ } .ci-status-icon-pending, +.ci-status-icon-failed_with_warnings, .ci-status-icon-success_with_warnings { color: $gl-warning; diff --git a/app/assets/stylesheets/pages/status.scss b/app/assets/stylesheets/pages/status.scss index f19275770be9df..6f31d4ed78977d 100644 --- a/app/assets/stylesheets/pages/status.scss +++ b/app/assets/stylesheets/pages/status.scss @@ -19,7 +19,8 @@ overflow: visible; } - &.ci-failed { + &.ci-failed, + &.ci-failed_with_warnings { color: $gl-danger; border-color: $gl-danger; -- GitLab From 227cbdd8ba969aecdbee68f1c892c85ed196d64e Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 12 Jan 2017 13:22:46 +0100 Subject: [PATCH 09/40] Use detailed status for failed but allowed builds --- lib/gitlab/ci/status/build/factory.rb | 7 +- .../gitlab/ci/status/build/factory_spec.rb | 125 ++++++++++++++++-- 2 files changed, 118 insertions(+), 14 deletions(-) diff --git a/lib/gitlab/ci/status/build/factory.rb b/lib/gitlab/ci/status/build/factory.rb index eee9a64120b287..38ac6edc9f188e 100644 --- a/lib/gitlab/ci/status/build/factory.rb +++ b/lib/gitlab/ci/status/build/factory.rb @@ -4,8 +4,11 @@ module Status module Build class Factory < Status::Factory def self.extended_statuses - [Status::Build::Stop, Status::Build::Play, - Status::Build::Cancelable, Status::Build::Retryable] + [[Status::Build::Cancelable, + Status::Build::Retryable], + [Status::Build::FailedAllowed, + Status::Build::Play, + Status::Build::Stop]] end def self.common_helpers diff --git a/spec/lib/gitlab/ci/status/build/factory_spec.rb b/spec/lib/gitlab/ci/status/build/factory_spec.rb index dccb29b5ef606b..0c40fca0c1a649 100644 --- a/spec/lib/gitlab/ci/status/build/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/build/factory_spec.rb @@ -3,15 +3,23 @@ describe Gitlab::Ci::Status::Build::Factory do let(:user) { create(:user) } let(:project) { build.project } - - subject { described_class.new(build, user) } - let(:status) { subject.fabricate! } + let(:status) { factory.fabricate! } + let(:factory) { described_class.new(build, user) } before { project.team << [user, :developer] } context 'when build is successful' do let(:build) { create(:ci_build, :success) } + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Success + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [Gitlab::Ci::Status::Build::Retryable] + end + it 'fabricates a retryable build status' do expect(status).to be_a Gitlab::Ci::Status::Build::Retryable end @@ -26,24 +34,72 @@ end context 'when build is failed' do - let(:build) { create(:ci_build, :failed) } + context 'when build is not allowed to fail' do + let(:build) { create(:ci_build, :failed) } - it 'fabricates a retryable build status' do - expect(status).to be_a Gitlab::Ci::Status::Build::Retryable + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Failed + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [Gitlab::Ci::Status::Build::Retryable] + end + + it 'fabricates a retryable build status' do + expect(status).to be_a Gitlab::Ci::Status::Build::Retryable + end + + it 'fabricates status with correct details' do + expect(status.text).to eq 'failed' + expect(status.icon).to eq 'icon_status_failed' + expect(status.label).to eq 'failed' + expect(status).to have_details + expect(status).to have_action + end end - it 'fabricates status with correct details' do - expect(status.text).to eq 'failed' - expect(status.icon).to eq 'icon_status_failed' - expect(status.label).to eq 'failed' - expect(status).to have_details - expect(status).to have_action + context 'when build is allowed to fail' do + let(:build) { create(:ci_build, :failed, :allowed_to_fail) } + + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Failed + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [Gitlab::Ci::Status::Build::Retryable, + Gitlab::Ci::Status::Build::FailedAllowed] + end + + it 'fabricates a failed but allowed build status' do + expect(status).to be_a Gitlab::Ci::Status::Build::FailedAllowed + end + + it 'fabricates status with correct details' do + expect(status.text).to eq 'failed' + expect(status.icon).to eq 'icon_status_warning' + expect(status.label).to eq 'failed (allowed to fail)' + expect(status).to have_details + expect(status).to have_action + expect(status.action_title).to include 'Retry' + expect(status.action_path).to include 'retry' + end end end context 'when build is a canceled' do let(:build) { create(:ci_build, :canceled) } + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Canceled + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [Gitlab::Ci::Status::Build::Retryable] + end + it 'fabricates a retryable build status' do expect(status).to be_a Gitlab::Ci::Status::Build::Retryable end @@ -60,6 +116,15 @@ context 'when build is running' do let(:build) { create(:ci_build, :running) } + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Running + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [Gitlab::Ci::Status::Build::Cancelable] + end + it 'fabricates a canceable build status' do expect(status).to be_a Gitlab::Ci::Status::Build::Cancelable end @@ -76,6 +141,15 @@ context 'when build is pending' do let(:build) { create(:ci_build, :pending) } + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Pending + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [Gitlab::Ci::Status::Build::Cancelable] + end + it 'fabricates a cancelable build status' do expect(status).to be_a Gitlab::Ci::Status::Build::Cancelable end @@ -92,6 +166,14 @@ context 'when build is skipped' do let(:build) { create(:ci_build, :skipped) } + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Skipped + end + + it 'does not match extended statuses' do + expect(factory.extended_statuses).to be_empty + end + it 'fabricates a core skipped status' do expect(status).to be_a Gitlab::Ci::Status::Skipped end @@ -109,6 +191,15 @@ context 'when build is a play action' do let(:build) { create(:ci_build, :playable) } + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Skipped + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [Gitlab::Ci::Status::Build::Play] + end + it 'fabricates a core skipped status' do expect(status).to be_a Gitlab::Ci::Status::Build::Play end @@ -119,12 +210,22 @@ expect(status.label).to eq 'manual play action' expect(status).to have_details expect(status).to have_action + expect(status.action_path).to include 'play' end end context 'when build is an environment stop action' do let(:build) { create(:ci_build, :playable, :teardown_environment) } + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Skipped + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [Gitlab::Ci::Status::Build::Stop] + end + it 'fabricates a core skipped status' do expect(status).to be_a Gitlab::Ci::Status::Build::Stop end -- GitLab From 528c06882560eb14d14babf5cf5bb73d2276cb0c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 12 Jan 2017 13:26:21 +0100 Subject: [PATCH 10/40] Fix a Rubocop offense in detailed status factory --- spec/lib/gitlab/ci/status/factory_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/gitlab/ci/status/factory_spec.rb b/spec/lib/gitlab/ci/status/factory_spec.rb index f1b758640a77a5..bbf9c7c83a3877 100644 --- a/spec/lib/gitlab/ci/status/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/factory_spec.rb @@ -11,7 +11,7 @@ let(:resource) { double('resource', status: simple_status) } let(:expected_status) do - Gitlab::Ci::Status.const_get(simple_status.capitalize) + Gitlab::Ci::Status.const_get(simple_status.capitalize) end it "fabricates a core status #{simple_status}" do -- GitLab From ee18d89f3dc2b00f7e9514e21c12139ecc8f9224 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 12 Jan 2017 13:46:53 +0100 Subject: [PATCH 11/40] Extend pipeline detailed status factory specs --- .../gitlab/ci/status/pipeline/factory_spec.rb | 45 ++++++++++++------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb b/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb index 0df6e881877545..b10a447c27a8da 100644 --- a/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/pipeline/factory_spec.rb @@ -3,29 +3,32 @@ describe Gitlab::Ci::Status::Pipeline::Factory do let(:user) { create(:user) } let(:project) { pipeline.project } - - subject do - described_class.new(pipeline, user) - end - - let(:status) do - subject.fabricate! - end + let(:status) { factory.fabricate! } + let(:factory) { described_class.new(pipeline, user) } before do project.team << [user, :developer] end context 'when pipeline has a core status' do - HasStatus::AVAILABLE_STATUSES.each do |core_status| - context "when core status is #{core_status}" do - let(:pipeline) do - create(:ci_pipeline, status: core_status) + HasStatus::AVAILABLE_STATUSES.each do |simple_status| + context "when core status is #{simple_status}" do + let(:pipeline) { create(:ci_pipeline, status: simple_status) } + + let(:expected_status) do + Gitlab::Ci::Status.const_get(simple_status.capitalize) + end + + it "matches correct core status for #{simple_status}" do + expect(factory.core_status).to be_a expected_status end - it "fabricates a core status #{core_status}" do - expect(status).to be_a( - Gitlab::Ci::Status.const_get(core_status.capitalize)) + it 'does not matche extended statuses' do + expect(factory.extended_statuses).to be_empty + end + + it "fabricates a core status #{simple_status}" do + expect(status).to be_a expected_status end it 'extends core status with common pipeline methods' do @@ -47,9 +50,17 @@ create(:ci_build, :allowed_to_fail, :failed, pipeline: pipeline) end + it 'matches correct core status' do + expect(factory.core_status).to be_a Gitlab::Ci::Status::Success + end + + it 'matches correct extended statuses' do + expect(factory.extended_statuses) + .to eq [Gitlab::Ci::Status::SuccessWarning] + end + it 'fabricates extended "success with warnings" status' do - expect(status) - .to be_a Gitlab::Ci::Status::SuccessWarning + expect(status).to be_a Gitlab::Ci::Status::SuccessWarning end it 'extends core status with common pipeline method' do -- GitLab From c3a940000ea20d6682313640e1a0fda9ff68dbdf Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Wed, 12 Oct 2016 19:07:36 +0200 Subject: [PATCH 12/40] Handles unsubscribe from notifications via email - allows unsubscription processing of email in format "reply+%{key}+unsubscribe@acme.com" (example) - if config.address includes %{key} and replies are enabled every unsubscriable message will include mailto: link in its List-Unsubscribe header --- app/mailers/notify.rb | 20 ++++-- ...ddress-to-unsubscribe-list-header-in-email | 4 ++ lib/gitlab/email/handler.rb | 3 +- lib/gitlab/email/handler/base_handler.rb | 43 +------------ .../email/handler/create_issue_handler.rb | 1 + .../email/handler/create_note_handler.rb | 7 ++- lib/gitlab/email/handler/reply_processing.rb | 54 ++++++++++++++++ .../email/handler/unsubscribe_handler.rb | 32 ++++++++++ lib/gitlab/incoming_email.rb | 9 ++- spec/lib/gitlab/email/email_shared_blocks.rb | 2 +- .../handler/create_issue_handler_spec.rb | 2 +- .../email/handler/create_note_handler_spec.rb | 2 +- .../email/handler/unsubscribe_handler_spec.rb | 61 +++++++++++++++++++ spec/lib/gitlab/incoming_email_spec.rb | 42 +++++++++++++ spec/support/notify_shared_examples.rb | 17 +++++- 15 files changed, 243 insertions(+), 56 deletions(-) create mode 100644 changelogs/unreleased/22619-add-an-email-address-to-unsubscribe-list-header-in-email create mode 100644 lib/gitlab/email/handler/reply_processing.rb create mode 100644 lib/gitlab/email/handler/unsubscribe_handler.rb create mode 100644 spec/lib/gitlab/email/handler/unsubscribe_handler_spec.rb diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index 0bc1c19e9cd3b5..0cd3456b4de90d 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -107,15 +107,11 @@ def message_id(model) def mail_thread(model, headers = {}) add_project_headers + add_unsubscription_headers_and_links + headers["X-GitLab-#{model.class.name}-ID"] = model.id headers['X-GitLab-Reply-Key'] = reply_key - if !@labels_url && @sent_notification && @sent_notification.unsubscribable? - headers['List-Unsubscribe'] = "<#{unsubscribe_sent_notification_url(@sent_notification, force: true)}>" - - @sent_notification_url = unsubscribe_sent_notification_url(@sent_notification) - end - if Gitlab::IncomingEmail.enabled? address = Mail::Address.new(Gitlab::IncomingEmail.reply_address(reply_key)) address.display_name = @project.name_with_namespace @@ -171,4 +167,16 @@ def add_project_headers headers['X-GitLab-Project-Id'] = @project.id headers['X-GitLab-Project-Path'] = @project.path_with_namespace end + + def add_unsubscription_headers_and_links + return unless !@labels_url && @sent_notification && @sent_notification.unsubscribable? + + list_unsubscribe_methods = [unsubscribe_sent_notification_url(@sent_notification, force: true)] + if Gitlab::IncomingEmail.enabled? && Gitlab::IncomingEmail.supports_wildcard? + list_unsubscribe_methods << "mailto:#{Gitlab::IncomingEmail.unsubscribe_address(reply_key)}" + end + + headers['List-Unsubscribe'] = list_unsubscribe_methods.map { |e| "<#{e}>" }.join(',') + @sent_notification_url = unsubscribe_sent_notification_url(@sent_notification) + end end diff --git a/changelogs/unreleased/22619-add-an-email-address-to-unsubscribe-list-header-in-email b/changelogs/unreleased/22619-add-an-email-address-to-unsubscribe-list-header-in-email new file mode 100644 index 00000000000000..f4011b756a521e --- /dev/null +++ b/changelogs/unreleased/22619-add-an-email-address-to-unsubscribe-list-header-in-email @@ -0,0 +1,4 @@ +--- +title: Handle unsubscribe from email notifications via replying to reply+%{key}+unsubscribe@ address +merge_request: 6597 +author: diff --git a/lib/gitlab/email/handler.rb b/lib/gitlab/email/handler.rb index bd3267e2a80ba5..bd2f5d3615eba9 100644 --- a/lib/gitlab/email/handler.rb +++ b/lib/gitlab/email/handler.rb @@ -1,10 +1,11 @@ require 'gitlab/email/handler/create_note_handler' require 'gitlab/email/handler/create_issue_handler' +require 'gitlab/email/handler/unsubscribe_handler' module Gitlab module Email module Handler - HANDLERS = [CreateNoteHandler, CreateIssueHandler] + HANDLERS = [UnsubscribeHandler, CreateNoteHandler, CreateIssueHandler] def self.for(mail, mail_key) HANDLERS.find do |klass| diff --git a/lib/gitlab/email/handler/base_handler.rb b/lib/gitlab/email/handler/base_handler.rb index 7cccf465334f5c..3f6ace0311a6e5 100644 --- a/lib/gitlab/email/handler/base_handler.rb +++ b/lib/gitlab/email/handler/base_handler.rb @@ -9,52 +9,13 @@ def initialize(mail, mail_key) @mail_key = mail_key end - def message - @message ||= process_message - end - - def author + def can_execute? raise NotImplementedError end - def project + def execute raise NotImplementedError end - - private - - def validate_permission!(permission) - raise UserNotFoundError unless author - raise UserBlockedError if author.blocked? - raise ProjectNotFound unless author.can?(:read_project, project) - raise UserNotAuthorizedError unless author.can?(permission, project) - end - - def process_message - message = ReplyParser.new(mail).execute.strip - add_attachments(message) - end - - def add_attachments(reply) - attachments = Email::AttachmentUploader.new(mail).execute(project) - - reply + attachments.map do |link| - "\n\n#{link[:markdown]}" - end.join - end - - def verify_record!(record:, invalid_exception:, record_name:) - return if record.persisted? - return if record.errors.key?(:commands_only) - - error_title = "The #{record_name} could not be created for the following reasons:" - - msg = error_title + record.errors.full_messages.map do |error| - "\n\n- #{error}" - end.join - - raise invalid_exception, msg - end end end end diff --git a/lib/gitlab/email/handler/create_issue_handler.rb b/lib/gitlab/email/handler/create_issue_handler.rb index 9f90a3ec2b2f70..127fae159d546b 100644 --- a/lib/gitlab/email/handler/create_issue_handler.rb +++ b/lib/gitlab/email/handler/create_issue_handler.rb @@ -5,6 +5,7 @@ module Gitlab module Email module Handler class CreateIssueHandler < BaseHandler + include ReplyProcessing attr_reader :project_path, :incoming_email_token def initialize(mail, mail_key) diff --git a/lib/gitlab/email/handler/create_note_handler.rb b/lib/gitlab/email/handler/create_note_handler.rb index 447c7a6a6b9465..d87ba427f4b7c0 100644 --- a/lib/gitlab/email/handler/create_note_handler.rb +++ b/lib/gitlab/email/handler/create_note_handler.rb @@ -1,10 +1,13 @@ require 'gitlab/email/handler/base_handler' +require 'gitlab/email/handler/reply_processing' module Gitlab module Email module Handler class CreateNoteHandler < BaseHandler + include ReplyProcessing + def can_handle? mail_key =~ /\A\w+\z/ end @@ -24,6 +27,8 @@ def execute record_name: 'comment') end + private + def author sent_notification.recipient end @@ -36,8 +41,6 @@ def sent_notification @sent_notification ||= SentNotification.for(mail_key) end - private - def create_note Notes::CreateService.new( project, diff --git a/lib/gitlab/email/handler/reply_processing.rb b/lib/gitlab/email/handler/reply_processing.rb new file mode 100644 index 00000000000000..32c5caf93e8118 --- /dev/null +++ b/lib/gitlab/email/handler/reply_processing.rb @@ -0,0 +1,54 @@ +module Gitlab + module Email + module Handler + module ReplyProcessing + private + + def author + raise NotImplementedError + end + + def project + raise NotImplementedError + end + + def message + @message ||= process_message + end + + def process_message + message = ReplyParser.new(mail).execute.strip + add_attachments(message) + end + + def add_attachments(reply) + attachments = Email::AttachmentUploader.new(mail).execute(project) + + reply + attachments.map do |link| + "\n\n#{link[:markdown]}" + end.join + end + + def validate_permission!(permission) + raise UserNotFoundError unless author + raise UserBlockedError if author.blocked? + raise ProjectNotFound unless author.can?(:read_project, project) + raise UserNotAuthorizedError unless author.can?(permission, project) + end + + def verify_record!(record:, invalid_exception:, record_name:) + return if record.persisted? + return if record.errors.key?(:commands_only) + + error_title = "The #{record_name} could not be created for the following reasons:" + + msg = error_title + record.errors.full_messages.map do |error| + "\n\n- #{error}" + end.join + + raise invalid_exception, msg + end + end + end + end +end diff --git a/lib/gitlab/email/handler/unsubscribe_handler.rb b/lib/gitlab/email/handler/unsubscribe_handler.rb new file mode 100644 index 00000000000000..97d7a8d65ff95e --- /dev/null +++ b/lib/gitlab/email/handler/unsubscribe_handler.rb @@ -0,0 +1,32 @@ +require 'gitlab/email/handler/base_handler' + +module Gitlab + module Email + module Handler + class UnsubscribeHandler < BaseHandler + def can_handle? + mail_key =~ /\A\w+#{Regexp.escape(Gitlab::IncomingEmail::UNSUBSCRIBE_SUFFIX)}\z/ + end + + def execute + raise SentNotificationNotFoundError unless sent_notification + return unless sent_notification.unsubscribable? + + noteable = sent_notification.noteable + raise NoteableNotFoundError unless noteable + noteable.unsubscribe(sent_notification.recipient) + end + + private + + def sent_notification + @sent_notification ||= SentNotification.for(reply_key) + end + + def reply_key + mail_key.sub(Gitlab::IncomingEmail::UNSUBSCRIBE_SUFFIX, '') + end + end + end + end +end diff --git a/lib/gitlab/incoming_email.rb b/lib/gitlab/incoming_email.rb index 801dfde9a368f9..b91012d6405b18 100644 --- a/lib/gitlab/incoming_email.rb +++ b/lib/gitlab/incoming_email.rb @@ -1,5 +1,6 @@ module Gitlab module IncomingEmail + UNSUBSCRIBE_SUFFIX = '+unsubscribe'.freeze WILDCARD_PLACEHOLDER = '%{key}'.freeze class << self @@ -18,7 +19,11 @@ def supports_issue_creation? end def reply_address(key) - config.address.gsub(WILDCARD_PLACEHOLDER, key) + config.address.sub(WILDCARD_PLACEHOLDER, key) + end + + def unsubscribe_address(key) + config.address.sub(WILDCARD_PLACEHOLDER, "#{key}#{UNSUBSCRIBE_SUFFIX}") end def key_from_address(address) @@ -49,7 +54,7 @@ def address_regex return nil unless wildcard_address regex = Regexp.escape(wildcard_address) - regex = regex.gsub(Regexp.escape('%{key}'), "(.+)") + regex = regex.sub(Regexp.escape(WILDCARD_PLACEHOLDER), '(.+)') Regexp.new(regex).freeze end end diff --git a/spec/lib/gitlab/email/email_shared_blocks.rb b/spec/lib/gitlab/email/email_shared_blocks.rb index 19298e261e3986..9d806fc524d0be 100644 --- a/spec/lib/gitlab/email/email_shared_blocks.rb +++ b/spec/lib/gitlab/email/email_shared_blocks.rb @@ -18,7 +18,7 @@ def setup_attachment end end -shared_examples :email_shared_examples do +shared_examples :reply_processing_shared_examples do context "when the user could not be found" do before do user.destroy diff --git a/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb b/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb index cb3651e3845be2..08897a4c310f16 100644 --- a/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb @@ -3,7 +3,7 @@ describe Gitlab::Email::Handler::CreateIssueHandler, lib: true do include_context :email_shared_context - it_behaves_like :email_shared_examples + it_behaves_like :reply_processing_shared_examples before do stub_incoming_email_setting(enabled: true, address: "incoming+%{key}@appmail.adventuretime.ooo") diff --git a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb index 48660d1dd1b035..cebbeff50cfb9c 100644 --- a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb @@ -3,7 +3,7 @@ describe Gitlab::Email::Handler::CreateNoteHandler, lib: true do include_context :email_shared_context - it_behaves_like :email_shared_examples + it_behaves_like :reply_processing_shared_examples before do stub_incoming_email_setting(enabled: true, address: "reply+%{key}@appmail.adventuretime.ooo") diff --git a/spec/lib/gitlab/email/handler/unsubscribe_handler_spec.rb b/spec/lib/gitlab/email/handler/unsubscribe_handler_spec.rb new file mode 100644 index 00000000000000..a444257754b58a --- /dev/null +++ b/spec/lib/gitlab/email/handler/unsubscribe_handler_spec.rb @@ -0,0 +1,61 @@ +require 'spec_helper' +require_relative '../email_shared_blocks' + +describe Gitlab::Email::Handler::UnsubscribeHandler, lib: true do + include_context :email_shared_context + + before do + stub_incoming_email_setting(enabled: true, address: 'reply+%{key}@appmail.adventuretime.ooo') + stub_config_setting(host: 'localhost') + end + + let(:email_raw) { fixture_file('emails/valid_reply.eml').gsub(mail_key, "#{mail_key}+unsubscribe") } + let(:project) { create(:project, :public) } + let(:user) { create(:user) } + let(:noteable) { create(:issue, project: project) } + + let!(:sent_notification) { SentNotification.record(noteable, user.id, mail_key) } + + context 'when notification concerns a commit' do + let(:commit) { create(:commit, project: project) } + let!(:sent_notification) { SentNotification.record(commit, user.id, mail_key) } + + it 'handler does not raise an error' do + expect { receiver.execute }.not_to raise_error + end + end + + context 'user is unsubscribed' do + it 'leaves user unsubscribed' do + expect { receiver.execute }.not_to change { noteable.subscribed?(user) }.from(false) + end + end + + context 'user is subscribed' do + before do + noteable.subscribe(user) + end + + it 'unsubscribes user from notable' do + expect { receiver.execute }.to change { noteable.subscribed?(user) }.from(true).to(false) + end + end + + context 'when the noteable could not be found' do + before do + noteable.destroy + end + + it 'raises a NoteableNotFoundError' do + expect { receiver.execute }.to raise_error(Gitlab::Email::NoteableNotFoundError) + end + end + + context 'when no sent notification for the mail key could be found' do + let(:email_raw) { fixture_file('emails/wrong_mail_key.eml') } + + it 'raises a SentNotificationNotFoundError' do + expect { receiver.execute }.to raise_error(Gitlab::Email::SentNotificationNotFoundError) + end + end +end diff --git a/spec/lib/gitlab/incoming_email_spec.rb b/spec/lib/gitlab/incoming_email_spec.rb index 1dcf2c0668b75f..7e951e3fcdd74b 100644 --- a/spec/lib/gitlab/incoming_email_spec.rb +++ b/spec/lib/gitlab/incoming_email_spec.rb @@ -23,6 +23,48 @@ end end + describe 'self.supports_wildcard?' do + context 'address contains the wildard placeholder' do + before do + stub_incoming_email_setting(address: 'replies+%{key}@example.com') + end + + it 'confirms that wildcard is supported' do + expect(described_class.supports_wildcard?).to be_truthy + end + end + + context "address doesn't contain the wildcard placeholder" do + before do + stub_incoming_email_setting(address: 'replies@example.com') + end + + it 'returns that wildcard is not supported' do + expect(described_class.supports_wildcard?).to be_falsey + end + end + + context 'address is not set' do + before do + stub_incoming_email_setting(address: nil) + end + + it 'returns that wildard is not supported' do + expect(described_class.supports_wildcard?).to be_falsey + end + end + end + + context 'self.unsubscribe_address' do + before do + stub_incoming_email_setting(address: 'replies+%{key}@example.com') + end + + it 'returns the address with interpolated reply key and unsubscribe suffix' do + expect(described_class.unsubscribe_address('key')).to eq('replies+key+unsubscribe@example.com') + end + end + context "self.reply_address" do before do stub_incoming_email_setting(address: "replies+%{key}@example.com") diff --git a/spec/support/notify_shared_examples.rb b/spec/support/notify_shared_examples.rb index 49867aa5cc4dbd..a3724b801b3f6e 100644 --- a/spec/support/notify_shared_examples.rb +++ b/spec/support/notify_shared_examples.rb @@ -179,9 +179,24 @@ end shared_examples 'an unsubscribeable thread' do + it_behaves_like 'an unsubscribeable thread with incoming address without %{key}' + + it 'has a List-Unsubscribe header in the correct format' do + is_expected.to have_header 'List-Unsubscribe', /unsubscribe/ + is_expected.to have_header 'List-Unsubscribe', /mailto/ + is_expected.to have_header 'List-Unsubscribe', /^<.+,.+>$/ + end + + it { is_expected.to have_body_text /unsubscribe/ } +end + +shared_examples 'an unsubscribeable thread with incoming address without %{key}' do + include_context 'reply-by-email is enabled with incoming address without %{key}' + it 'has a List-Unsubscribe header in the correct format' do is_expected.to have_header 'List-Unsubscribe', /unsubscribe/ - is_expected.to have_header 'List-Unsubscribe', /^<.+>$/ + is_expected.not_to have_header 'List-Unsubscribe', /mailto/ + is_expected.to have_header 'List-Unsubscribe', /^<[^,]+>$/ end it { is_expected.to have_body_text /unsubscribe/ } -- GitLab From eb9b96405498e37b25aa32876b0e101d1880f4e9 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 5 Jan 2017 12:40:54 +0100 Subject: [PATCH 13/40] Allow creating protected branch when it doesn't exist if user has either push or merge permissions + Change log entry for fix to creating a branch matching a wildcard fails --- ...22638-creating-a-branch-matching-a-wildcard-fails.yml | 4 ++++ lib/gitlab/user_access.rb | 4 +++- spec/lib/gitlab/user_access_spec.rb | 9 ++++++++- 3 files changed, 15 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/22638-creating-a-branch-matching-a-wildcard-fails.yml diff --git a/changelogs/unreleased/22638-creating-a-branch-matching-a-wildcard-fails.yml b/changelogs/unreleased/22638-creating-a-branch-matching-a-wildcard-fails.yml new file mode 100644 index 00000000000000..2c6883bcf7b5d7 --- /dev/null +++ b/changelogs/unreleased/22638-creating-a-branch-matching-a-wildcard-fails.yml @@ -0,0 +1,4 @@ +--- +title: Allow creating protected branches when user can merge to such branch +merge_request: 8458 +author: diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 6c7e673fb9fbf3..6ce9b229294355 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -35,7 +35,9 @@ def can_push_to_branch?(ref) return true if project.empty_repo? && project.user_can_push_to_empty_repo?(user) access_levels = project.protected_branches.matching(ref).map(&:push_access_levels).flatten - access_levels.any? { |access_level| access_level.check_access(user) } + has_access = access_levels.any? { |access_level| access_level.check_access(user) } + + has_access || !project.repository.branch_exists?(ref) && can_merge_to_branch?(ref) else user.can?(:push_code, project) end diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb index d3c3b800b94b36..369e55f61f1d49 100644 --- a/spec/lib/gitlab/user_access_spec.rb +++ b/spec/lib/gitlab/user_access_spec.rb @@ -66,7 +66,8 @@ end describe 'push to protected branch' do - let(:branch) { create :protected_branch, project: project } + let(:branch) { create :protected_branch, project: project, name: "test" } + let(:not_existing_branch) { create :protected_branch, :developers_can_merge, project: project } it 'returns true if user is a master' do project.team << [user, :master] @@ -85,6 +86,12 @@ expect(access.can_push_to_branch?(branch.name)).to be_falsey end + + it 'returns true if branch does not exist and user has permission to merge' do + project.team << [user, :developer] + + expect(access.can_push_to_branch?(not_existing_branch.name)).to be_truthy + end end describe 'push to protected branch if allowed for developers' do -- GitLab From e2585e642ac53802c6c5b4c2ee2ec3e6be584981 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 13 Jan 2017 16:12:02 +0000 Subject: [PATCH 14/40] Rename endboss -> maintainer, miniboss -> reviewer We want to describe these roles in a way that is more understandable to people not familiar with GitLab. --- doc/development/code_review.md | 12 ++++++------ .../merge_request_performance_guidelines.md | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 1ef34c79971b96..e4a0e0b92bc43b 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -9,7 +9,7 @@ code is effective, understandable, and maintainable. Any developer can, and is encouraged to, perform code review on merge requests of colleagues and contributors. However, the final decision to accept a merge -request is up to one of our merge request "endbosses", denoted on the +request is up to one the project's maintainers, denoted on the [team page](https://about.gitlab.com/team). ## Everyone @@ -81,15 +81,15 @@ balance in how deep the reviewer can interfere with the code created by a reviewee. - Learning how to find the right balance takes time; that is why we have - minibosses that become merge request endbosses after some time spent on - reviewing merge requests. + reviewers that become maintainers after some time spent on reviewing merge + requests. - Finding bugs and improving code style is important, but thinking about good design is important as well. Building abstractions and good design is what makes it possible to hide complexity and makes future changes easier. - Asking the reviewee to change the design sometimes means the complete rewrite - of the contributed code. It's usually a good idea to ask another merge - request endboss before doing it, but have the courage to do it when you - believe it is important. + of the contributed code. It's usually a good idea to ask another maintainer or + reviewer before doing it, but have the courage to do it when you believe it is + important. - There is a difference in doing things right and doing things right now. Ideally, we should do the former, but in the real world we need the latter as well. A good example is a security fix which should be released as soon as diff --git a/doc/development/merge_request_performance_guidelines.md b/doc/development/merge_request_performance_guidelines.md index 0363bf8c1d54b1..8232a0a113cbdd 100644 --- a/doc/development/merge_request_performance_guidelines.md +++ b/doc/development/merge_request_performance_guidelines.md @@ -3,7 +3,7 @@ To ensure a merge request does not negatively impact performance of GitLab _every_ merge request **must** adhere to the guidelines outlined in this document. There are no exceptions to this rule unless specifically discussed -with and agreed upon by merge request endbosses and performance specialists. +with and agreed upon by backend maintainers and performance specialists. To measure the impact of a merge request you can use [Sherlock](profiling.md#sherlock). It's also highly recommended that you read @@ -40,9 +40,9 @@ section below for more information. about the impact. Sometimes it's hard to assess the impact of a merge request. In this case you -should ask one of the merge request (mini) endbosses to review your changes. You -can find a list of these endbosses at . An -endboss in turn can request a performance specialist to review the changes. +should ask one of the merge request reviewers to review your changes. You can +find a list of these reviewers at . A reviewer +in turn can request a performance specialist to review the changes. ## Query Counts -- GitLab From cff9c16be724398e3d6e7170cc9f5e39cfd8cdb7 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 18 Jan 2017 11:07:12 +0100 Subject: [PATCH 15/40] Pass memoizable warnings attribute to stage object --- app/models/ci/pipeline.rb | 15 ++++++++++----- app/models/ci/stage.rb | 5 +++-- spec/factories/ci/stages.rb | 3 ++- spec/models/ci/stage_spec.rb | 20 +++++++++++++++----- 4 files changed, 30 insertions(+), 13 deletions(-) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 2a97e8bae4a8ea..fab8497ec7db82 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -128,16 +128,21 @@ def stages_name end def stages + # TODO, this needs refactoring, see gitlab-ce#26481. + + stages_query = statuses + .group('stage').select(:stage).order('max(stage_idx)') + status_sql = statuses.latest.where('stage=sg.stage').status_sql - stages_query = statuses.group('stage').select(:stage) - .order('max(stage_idx)') + warnings_sql = statuses.latest.select('COUNT(*) > 0') + .where('stage=sg.stage').failed_but_allowed.to_sql - stages_with_statuses = CommitStatus.from(stages_query, :sg). - pluck('sg.stage', status_sql) + stages_with_statuses = CommitStatus.from(stages_query, :sg) + .pluck('sg.stage', status_sql, "(#{warnings_sql})") stages_with_statuses.map do |stage| - Ci::Stage.new(self, name: stage.first, status: stage.last) + Ci::Stage.new(self, Hash[%i[name status warnings].zip(stage)]) end end diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index d4b6ff910aa4d4..ca76d82f358367 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -8,10 +8,11 @@ class Stage delegate :project, to: :pipeline - def initialize(pipeline, name:, status: nil) + def initialize(pipeline, name:, status: nil, warnings: nil) @pipeline = pipeline @name = name @status = status + @warnings = warnings end def to_param @@ -45,7 +46,7 @@ def success? end def has_warnings? - statuses.latest.failed_but_allowed.any? + @warnings ||= statuses.latest.failed_but_allowed.any? end end end diff --git a/spec/factories/ci/stages.rb b/spec/factories/ci/stages.rb index ee3b17b8bf1fe0..7f557b25ccbb11 100644 --- a/spec/factories/ci/stages.rb +++ b/spec/factories/ci/stages.rb @@ -3,11 +3,12 @@ transient do name 'test' status nil + warnings nil pipeline factory: :ci_empty_pipeline end initialize_with do - Ci::Stage.new(pipeline, name: name, status: status) + Ci::Stage.new(pipeline, name: name, status: status, warnings: warnings) end end end diff --git a/spec/models/ci/stage_spec.rb b/spec/models/ci/stage_spec.rb index 3d387d52c8e720..cca0cb1e3b0090 100644 --- a/spec/models/ci/stage_spec.rb +++ b/spec/models/ci/stage_spec.rb @@ -168,13 +168,23 @@ describe '#has_warnings?' do context 'when stage has warnings' do - before do - create(:ci_build, :failed, :allowed_to_fail, - stage: stage_name, pipeline: pipeline) + context 'when using memoized warnings flag' do + let(:stage) { build(:ci_stage, warnings: true) } + + it 'has warnings' do + expect(stage).to have_warnings + end end - it 'has warnings' do - expect(stage).to have_warnings + context 'when calculating warnings from statuses' do + before do + create(:ci_build, :failed, :allowed_to_fail, + stage: stage_name, pipeline: pipeline) + end + + it 'has warnings' do + expect(stage).to have_warnings + end end end -- GitLab From e66b0414cb0a81565937391252e841c777bf270b Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 18 Jan 2017 11:25:36 +0100 Subject: [PATCH 16/40] Improve readability of specs for pipeline stages --- spec/models/ci/pipeline_spec.rb | 103 ++++++++++++++++++++------------ 1 file changed, 64 insertions(+), 39 deletions(-) diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index d1aee27057abe3..2bdd611aeed41b 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -122,55 +122,80 @@ def create_build(name, status) end end - describe '#stages' do + describe 'pipeline stages' do before do - create(:commit_status, pipeline: pipeline, stage: 'build', name: 'linux', stage_idx: 0, status: 'success') - create(:commit_status, pipeline: pipeline, stage: 'build', name: 'mac', stage_idx: 0, status: 'failed') - create(:commit_status, pipeline: pipeline, stage: 'deploy', name: 'staging', stage_idx: 2, status: 'running') - create(:commit_status, pipeline: pipeline, stage: 'test', name: 'rspec', stage_idx: 1, status: 'success') - end - - subject { pipeline.stages } - - context 'stages list' do - it 'returns ordered list of stages' do - expect(subject.map(&:name)).to eq(%w[build test deploy]) + create(:commit_status, pipeline: pipeline, + stage: 'build', + name: 'linux', + stage_idx: 0, + status: 'success') + + create(:commit_status, pipeline: pipeline, + stage: 'build', + name: 'mac', + stage_idx: 0, + status: 'failed') + + create(:commit_status, pipeline: pipeline, + stage: 'deploy', + name: 'staging', + stage_idx: 2, + status: 'running') + + create(:commit_status, pipeline: pipeline, + stage: 'test', + name: 'rspec', + stage_idx: 1, + status: 'success') + end + + describe '#stages' do + subject { pipeline.stages } + + context 'stages list' do + it 'returns ordered list of stages' do + expect(subject.map(&:name)).to eq(%w[build test deploy]) + end end - end - it 'returns a valid number of stages' do - expect(pipeline.stages_count).to eq(3) - end + context 'stages with statuses' do + let(:statuses) do + subject.map { |stage| [stage.name, stage.status] } + end - it 'returns a valid names of stages' do - expect(pipeline.stages_name).to eq(['build', 'test', 'deploy']) - end + it 'returns list of stages with correct statuses' do + expect(statuses).to eq([['build', 'failed'], + ['test', 'success'], + ['deploy', 'running']]) + end - context 'stages with statuses' do - let(:statuses) do - subject.map do |stage| - [stage.name, stage.status] + context 'when commit status is retried' do + before do + create(:commit_status, pipeline: pipeline, + stage: 'build', + name: 'mac', + stage_idx: 0, + status: 'success') + end + + it 'ignores the previous state' do + expect(statuses).to eq([['build', 'success'], + ['test', 'success'], + ['deploy', 'running']]) + end end end + end - it 'returns list of stages with statuses' do - expect(statuses).to eq([['build', 'failed'], - ['test', 'success'], - ['deploy', 'running'] - ]) + describe '#stages_count' do + it 'returns a valid number of stages' do + expect(pipeline.stages_count).to eq(3) end + end - context 'when build is retried' do - before do - create(:commit_status, pipeline: pipeline, stage: 'build', name: 'mac', stage_idx: 0, status: 'success') - end - - it 'ignores the previous state' do - expect(statuses).to eq([['build', 'success'], - ['test', 'success'], - ['deploy', 'running'] - ]) - end + describe '#stages_name' do + it 'returns a valid names of stages' do + expect(pipeline.stages_name).to eq(['build', 'test', 'deploy']) end end end -- GitLab From 3bc0525ad90aa84ff864a0bc9c43e18ec5d5cd3d Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 18 Jan 2017 11:30:28 +0100 Subject: [PATCH 17/40] Extract compound statuses method in status factory --- lib/gitlab/ci/status/factory.rb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/ci/status/factory.rb b/lib/gitlab/ci/status/factory.rb index 3da5ae4fc01983..4d7c71ed469fde 100644 --- a/lib/gitlab/ci/status/factory.rb +++ b/lib/gitlab/ci/status/factory.rb @@ -12,9 +12,7 @@ def fabricate! if extended_statuses.none? core_status else - extended_statuses.inject(core_status) do |status, extended| - extended.new(status) - end + compound_extended_status end end @@ -25,6 +23,12 @@ def core_status .extend(self.class.common_helpers) end + def compound_extended_status + extended_statuses.inject(core_status) do |status, extended| + extended.new(status) + end + end + def extended_statuses return @extended_statuses if defined?(@extended_statuses) -- GitLab From 73fcfb296c90746f868ec11a19477a16039ef9a5 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 18 Jan 2017 11:34:55 +0100 Subject: [PATCH 18/40] Add a default status const to common status concern --- app/models/concerns/has_status.rb | 1 + lib/gitlab/ci/status/factory.rb | 2 +- spec/models/concerns/has_status_spec.rb | 6 ++++++ 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb index 90432fc4050ac0..431c035496917a 100644 --- a/app/models/concerns/has_status.rb +++ b/app/models/concerns/has_status.rb @@ -1,6 +1,7 @@ module HasStatus extend ActiveSupport::Concern + DEFAULT_STATUS = 'created' AVAILABLE_STATUSES = %w[created pending running success failed canceled skipped] STARTED_STATUSES = %w[running success failed skipped] ACTIVE_STATUSES = %w[pending running] diff --git a/lib/gitlab/ci/status/factory.rb b/lib/gitlab/ci/status/factory.rb index 4d7c71ed469fde..15836c699c7ce6 100644 --- a/lib/gitlab/ci/status/factory.rb +++ b/lib/gitlab/ci/status/factory.rb @@ -5,7 +5,7 @@ class Factory def initialize(subject, user) @subject = subject @user = user - @status = subject.status || :created + @status = subject.status || HasStatus::DEFAULT_STATUS end def fabricate! diff --git a/spec/models/concerns/has_status_spec.rb b/spec/models/concerns/has_status_spec.rb index 4d0f51fe82a3ec..dbfe3cd2d3631e 100644 --- a/spec/models/concerns/has_status_spec.rb +++ b/spec/models/concerns/has_status_spec.rb @@ -219,4 +219,10 @@ end end end + + describe '::DEFAULT_STATUS' do + it 'is a status created' do + expect(described_class::DEFAULT_STATUS).to eq 'created' + end + end end -- GitLab From 86217866fda66cb770ee8c9a540bf535a980eb36 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 19 Jan 2017 18:49:14 +0100 Subject: [PATCH 19/40] Fix warnings argument memoization in CI/CD stage --- app/models/ci/stage.rb | 6 +++++- spec/models/ci/stage_spec.rb | 24 +++++++++++++++++++----- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index ca76d82f358367..ca74c91b0627b5 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -46,7 +46,11 @@ def success? end def has_warnings? - @warnings ||= statuses.latest.failed_but_allowed.any? + if @warnings.nil? + statuses.latest.failed_but_allowed.any? + else + @warnings + end end end end diff --git a/spec/models/ci/stage_spec.rb b/spec/models/ci/stage_spec.rb index cca0cb1e3b0090..c4a9743a4e245b 100644 --- a/spec/models/ci/stage_spec.rb +++ b/spec/models/ci/stage_spec.rb @@ -169,10 +169,22 @@ describe '#has_warnings?' do context 'when stage has warnings' do context 'when using memoized warnings flag' do - let(:stage) { build(:ci_stage, warnings: true) } + context 'when there are warnings' do + let(:stage) { build(:ci_stage, warnings: true) } - it 'has warnings' do - expect(stage).to have_warnings + it 'has memoized warnings' do + expect(stage).not_to receive(:statuses) + expect(stage).to have_warnings + end + end + + context 'when there are no warnings' do + let(:stage) { build(:ci_stage, warnings: false) } + + it 'has memoized warnings' do + expect(stage).not_to receive(:statuses) + expect(stage).not_to have_warnings + end end end @@ -182,7 +194,8 @@ stage: stage_name, pipeline: pipeline) end - it 'has warnings' do + it 'has warnings calculated from statuses' do + expect(stage).to receive(:statuses).and_call_original expect(stage).to have_warnings end end @@ -194,7 +207,8 @@ pipeline: pipeline) end - it 'does not have warnings' do + it 'does not have warnings calculated from statuses' do + expect(stage).to receive(:statuses).and_call_original expect(stage).not_to have_warnings end end -- GitLab From eed6bfe2e5179f4d8d07c244d2b2f880da8e183f Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 19 Jan 2017 22:45:50 -0600 Subject: [PATCH 20/40] Remove rogue scrollbars for some issue comments Fix https://gitlab.com/gitlab-org/gitlab-ce/issues/25989 --- app/assets/stylesheets/pages/notes.scss | 4 ++-- .../unreleased/25989-fix-rogue-scrollbars-on-comments.yml | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/25989-fix-rogue-scrollbars-on-comments.yml diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index cbe38b60d60b42..da0caa30c26734 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -195,10 +195,10 @@ ul.notes { } .note-body { - overflow: auto; + overflow-x: auto; + overflow-y: hidden; .note-text { - overflow: auto; word-wrap: break-word; @include md-typography; // Reset ul style types since we're nested inside a ul already diff --git a/changelogs/unreleased/25989-fix-rogue-scrollbars-on-comments.yml b/changelogs/unreleased/25989-fix-rogue-scrollbars-on-comments.yml new file mode 100644 index 00000000000000..e67a9c0da152b1 --- /dev/null +++ b/changelogs/unreleased/25989-fix-rogue-scrollbars-on-comments.yml @@ -0,0 +1,4 @@ +--- +title: Remove rogue scrollbars for issue comments with inline elements +merge_request: +author: -- GitLab From 26d109ea01660bbb909525ed7f9cb8920fca300c Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Fri, 20 Jan 2017 11:29:11 +0100 Subject: [PATCH 21/40] Remove prefilled prefix Fixes, at least partially, gitlab-org/gitlab-ce#26755. --- app/models/project_services/mattermost_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/project_services/mattermost_service.rb b/app/models/project_services/mattermost_service.rb index ee8a0b5527561b..2bea3827b952bb 100644 --- a/app/models/project_services/mattermost_service.rb +++ b/app/models/project_services/mattermost_service.rb @@ -36,6 +36,6 @@ def default_fields end def default_channel_placeholder - "#town-square" + "town-square" end end -- GitLab From 607b3c4892578020bd0f7dcd669fd4fc68430cfc Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Fri, 20 Jan 2017 11:31:42 +0100 Subject: [PATCH 22/40] Fix Slack/Mattermost notifaction services --- .../project_services/chat_notification_service.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/app/models/project_services/chat_notification_service.rb b/app/models/project_services/chat_notification_service.rb index b7ef44c3054523..3098625b0c236d 100644 --- a/app/models/project_services/chat_notification_service.rb +++ b/app/models/project_services/chat_notification_service.rb @@ -82,19 +82,19 @@ def default_channel_placeholder def get_message(object_kind, data) case object_kind when "push", "tag_push" - PushMessage.new(data) + ChatMessage::PushMessage.new(data) when "issue" - IssueMessage.new(data) unless is_update?(data) + ChatMessage::IssueMessage.new(data) unless is_update?(data) when "merge_request" - MergeMessage.new(data) unless is_update?(data) + ChatMessage::MergeMessage.new(data) unless is_update?(data) when "note" - NoteMessage.new(data) + ChatMessage::NoteMessage.new(data) when "build" - BuildMessage.new(data) if should_build_be_notified?(data) + ChatMessage::BuildMessage.new(data) if should_build_be_notified?(data) when "pipeline" - PipelineMessage.new(data) if should_pipeline_be_notified?(data) + ChatMessage::PipelineMessage.new(data) if should_pipeline_be_notified?(data) when "wiki_page" - WikiPageMessage.new(data) + ChatMessage::WikiPageMessage.new(data) end end -- GitLab From cd51af1a991eaa5e16e3c6b94c89d90e20a5641f Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Tue, 27 Dec 2016 12:44:24 +0000 Subject: [PATCH 23/40] adds events to services api deserialization --- app/helpers/services_helper.rb | 18 +++---- app/models/project_services/asana_service.rb | 8 ++- .../project_services/assembla_service.rb | 8 ++- app/models/project_services/bamboo_service.rb | 4 +- .../project_services/bugzilla_service.rb | 10 +++- .../project_services/buildkite_service.rb | 8 ++- .../project_services/builds_email_service.rb | 8 ++- .../project_services/campfire_service.rb | 8 ++- .../chat_notification_service.rb | 2 +- .../chat_slash_commands_service.rb | 4 +- app/models/project_services/ci_service.rb | 5 +- .../custom_issue_tracker_service.rb | 10 +++- .../project_services/deployment_service.rb | 4 +- .../project_services/drone_ci_service.rb | 4 +- .../emails_on_push_service.rb | 8 ++- .../project_services/external_wiki_service.rb | 10 +++- .../project_services/flowdock_service.rb | 8 ++- .../project_services/gemnasium_service.rb | 8 ++- .../gitlab_issue_tracker_service.rb | 2 +- .../project_services/hipchat_service.rb | 8 ++- app/models/project_services/irker_service.rb | 8 ++- .../project_services/issue_tracker_service.rb | 2 +- app/models/project_services/jira_service.rb | 8 ++- .../project_services/kubernetes_service.rb | 10 +++- .../project_services/mattermost_service.rb | 10 +++- .../mattermost_slash_commands_service.rb | 10 +++- .../pipelines_email_service.rb | 8 ++- .../pivotaltracker_service.rb | 8 ++- .../project_services/pushover_service.rb | 8 ++- .../project_services/redmine_service.rb | 10 +++- app/models/project_services/slack_service.rb | 10 +++- .../slack_slash_commands_service.rb | 10 +++- .../project_services/teamcity_service.rb | 8 ++- app/models/service.rb | 6 ++- ...974-trigger-service-events-through-api.yml | 4 ++ lib/api/services.rb | 49 +++++++++++++++++-- spec/requests/api/services_spec.rb | 11 ++++- 37 files changed, 259 insertions(+), 66 deletions(-) create mode 100644 changelogs/unreleased/22974-trigger-service-events-through-api.yml diff --git a/app/helpers/services_helper.rb b/app/helpers/services_helper.rb index 9bab140e60abc1..790ea446e673ef 100644 --- a/app/helpers/services_helper.rb +++ b/app/helpers/services_helper.rb @@ -1,23 +1,23 @@ module ServicesHelper def service_event_description(event) case event - when "push" + when "push", "push_events" "Event will be triggered by a push to the repository" - when "tag_push" + when "tag_push", "tag_push_events" "Event will be triggered when a new tag is pushed to the repository" - when "note" + when "note", "note_events" "Event will be triggered when someone adds a comment" - when "issue" + when "issue", "issue_events" "Event will be triggered when an issue is created/updated/closed" - when "confidential_issue" + when "confidential_issue", "confidential_issue_events" "Event will be triggered when a confidential issue is created/updated/closed" - when "merge_request" + when "merge_request", "merge_request_events" "Event will be triggered when a merge request is created/updated/merged" - when "build" + when "build", "build_events" "Event will be triggered when a build status changes" - when "wiki_page" + when "wiki_page", "wiki_page_events" "Event will be triggered when a wiki page is created/updated" - when "commit" + when "commit", "commit_events" "Event will be triggered when a commit is created/updated" end end diff --git a/app/models/project_services/asana_service.rb b/app/models/project_services/asana_service.rb index 7c23b76676374c..20cf7dc4632d55 100644 --- a/app/models/project_services/asana_service.rb +++ b/app/models/project_services/asana_service.rb @@ -25,7 +25,7 @@ def help http://app.asana.com/-/account_api' end - def to_param + def self.to_param 'asana' end @@ -44,10 +44,14 @@ def fields ] end - def supported_events + def self.supported_events %w(push) end + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end + def client @_client ||= begin Asana::Client.new do |c| diff --git a/app/models/project_services/assembla_service.rb b/app/models/project_services/assembla_service.rb index d839221d315e04..f18d2204e0e1b3 100644 --- a/app/models/project_services/assembla_service.rb +++ b/app/models/project_services/assembla_service.rb @@ -12,7 +12,7 @@ def description 'Project Management Software (Source Commits Endpoint)' end - def to_param + def self.to_param 'assembla' end @@ -23,10 +23,14 @@ def fields ] end - def supported_events + def self.supported_events %w(push) end + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end + def execute(data) return unless supported_events.include?(data[:object_kind]) diff --git a/app/models/project_services/bamboo_service.rb b/app/models/project_services/bamboo_service.rb index 4819bdbef8cae0..fe95fc14525e90 100644 --- a/app/models/project_services/bamboo_service.rb +++ b/app/models/project_services/bamboo_service.rb @@ -40,7 +40,7 @@ def help 'You must set up automatic revision labeling and a repository trigger in Bamboo.' end - def to_param + def self.to_param 'bamboo' end @@ -56,7 +56,7 @@ def fields ] end - def supported_events + def self.supported_events %w(push) end diff --git a/app/models/project_services/bugzilla_service.rb b/app/models/project_services/bugzilla_service.rb index 338e685339a03c..18c0cdf2db80eb 100644 --- a/app/models/project_services/bugzilla_service.rb +++ b/app/models/project_services/bugzilla_service.rb @@ -19,7 +19,15 @@ def description end end - def to_param + def self.to_param 'bugzilla' end + + def self.supported_events + %w() + end + + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end end diff --git a/app/models/project_services/buildkite_service.rb b/app/models/project_services/buildkite_service.rb index e77942d8f3cff0..94bc179decaecd 100644 --- a/app/models/project_services/buildkite_service.rb +++ b/app/models/project_services/buildkite_service.rb @@ -24,10 +24,14 @@ def compose_service_hook hook.save end - def supported_events + def self.supported_events %w(push) end + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end + def execute(data) return unless supported_events.include?(data[:object_kind]) @@ -54,7 +58,7 @@ def description 'Continuous integration and deployments' end - def to_param + def self.to_param 'buildkite' end diff --git a/app/models/project_services/builds_email_service.rb b/app/models/project_services/builds_email_service.rb index 201b94b065ba89..51c07f731ffe25 100644 --- a/app/models/project_services/builds_email_service.rb +++ b/app/models/project_services/builds_email_service.rb @@ -19,14 +19,18 @@ def description 'Email the builds status to a list of recipients.' end - def to_param + def self.to_param 'builds_email' end - def supported_events + def self.supported_events %w(build) end + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end + def execute(push_data) return unless supported_events.include?(push_data[:object_kind]) return unless should_build_be_notified?(push_data) diff --git a/app/models/project_services/campfire_service.rb b/app/models/project_services/campfire_service.rb index 5af93860d09df9..52371cde20152b 100644 --- a/app/models/project_services/campfire_service.rb +++ b/app/models/project_services/campfire_service.rb @@ -12,7 +12,7 @@ def description 'Simple web-based real-time group chat' end - def to_param + def self.to_param 'campfire' end @@ -24,10 +24,14 @@ def fields ] end - def supported_events + def self.supported_events %w(push) end + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end + def execute(data) return unless supported_events.include?(data[:object_kind]) diff --git a/app/models/project_services/chat_notification_service.rb b/app/models/project_services/chat_notification_service.rb index b7ef44c3054523..c781c3fdb3426b 100644 --- a/app/models/project_services/chat_notification_service.rb +++ b/app/models/project_services/chat_notification_service.rb @@ -25,7 +25,7 @@ def can_test? valid? end - def supported_events + def self.supported_events %w[push issue confidential_issue merge_request note tag_push build pipeline wiki_page] end diff --git a/app/models/project_services/chat_slash_commands_service.rb b/app/models/project_services/chat_slash_commands_service.rb index 0bc160af604263..2bcff541cc0967 100644 --- a/app/models/project_services/chat_slash_commands_service.rb +++ b/app/models/project_services/chat_slash_commands_service.rb @@ -13,8 +13,8 @@ def valid_token?(token) ActiveSupport::SecurityUtils.variable_size_secure_compare(token, self.token) end - def supported_events - [] + def self.supported_events + %w() end def can_test? diff --git a/app/models/project_services/ci_service.rb b/app/models/project_services/ci_service.rb index 4de0106707e644..9b470c3a07811b 100644 --- a/app/models/project_services/ci_service.rb +++ b/app/models/project_services/ci_service.rb @@ -8,10 +8,13 @@ def valid_token?(token) self.respond_to?(:token) && self.token.present? && ActiveSupport::SecurityUtils.variable_size_secure_compare(token, self.token) end - def supported_events + def self.supported_events %w(push) end + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end # Return complete url to build page # # Ex. diff --git a/app/models/project_services/custom_issue_tracker_service.rb b/app/models/project_services/custom_issue_tracker_service.rb index b2f426dc2acc4e..9c5c86b1cc9318 100644 --- a/app/models/project_services/custom_issue_tracker_service.rb +++ b/app/models/project_services/custom_issue_tracker_service.rb @@ -23,7 +23,7 @@ def description end end - def to_param + def self.to_param 'custom_issue_tracker' end @@ -36,4 +36,12 @@ def fields { type: 'text', name: 'new_issue_url', placeholder: 'New Issue url' } ] end + + def self.supported_events + %w() + end + + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end end diff --git a/app/models/project_services/deployment_service.rb b/app/models/project_services/deployment_service.rb index ab353a1abe61c2..91a55514a9a64d 100644 --- a/app/models/project_services/deployment_service.rb +++ b/app/models/project_services/deployment_service.rb @@ -5,8 +5,8 @@ class DeploymentService < Service default_value_for :category, 'deployment' - def supported_events - [] + def self.supported_events + %w() end def predefined_variables diff --git a/app/models/project_services/drone_ci_service.rb b/app/models/project_services/drone_ci_service.rb index 4bbbebf54cba17..0a217d8cabab83 100644 --- a/app/models/project_services/drone_ci_service.rb +++ b/app/models/project_services/drone_ci_service.rb @@ -32,7 +32,7 @@ def allow_target_ci? true end - def supported_events + def self.supported_events %w(push merge_request tag_push) end @@ -87,7 +87,7 @@ def description 'Drone is a Continuous Integration platform built on Docker, written in Go' end - def to_param + def self.to_param 'drone_ci' end diff --git a/app/models/project_services/emails_on_push_service.rb b/app/models/project_services/emails_on_push_service.rb index 79285cbd26de11..23dc0bc3790f70 100644 --- a/app/models/project_services/emails_on_push_service.rb +++ b/app/models/project_services/emails_on_push_service.rb @@ -12,14 +12,18 @@ def description 'Email the commits and diff of each push to a list of recipients.' end - def to_param + def self.to_param 'emails_on_push' end - def supported_events + def self.supported_events %w(push tag_push) end + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end + def execute(push_data) return unless supported_events.include?(push_data[:object_kind]) diff --git a/app/models/project_services/external_wiki_service.rb b/app/models/project_services/external_wiki_service.rb index d7b6e505191b8c..961b1c19a33b93 100644 --- a/app/models/project_services/external_wiki_service.rb +++ b/app/models/project_services/external_wiki_service.rb @@ -13,7 +13,7 @@ def description 'Replaces the link to the internal wiki with a link to an external wiki.' end - def to_param + def self.to_param 'external_wiki' end @@ -29,4 +29,12 @@ def execute(_data) nil end end + + def self.supported_events + %w() + end + + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end end diff --git a/app/models/project_services/flowdock_service.rb b/app/models/project_services/flowdock_service.rb index dd00275187f635..00a15d12efa30f 100644 --- a/app/models/project_services/flowdock_service.rb +++ b/app/models/project_services/flowdock_service.rb @@ -12,7 +12,7 @@ def description 'Flowdock is a collaboration web app for technical teams.' end - def to_param + def self.to_param 'flowdock' end @@ -22,10 +22,14 @@ def fields ] end - def supported_events + def self.supported_events %w(push) end + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end + def execute(data) return unless supported_events.include?(data[:object_kind]) diff --git a/app/models/project_services/gemnasium_service.rb b/app/models/project_services/gemnasium_service.rb index 598aca5e06d28b..b70d74e75c1a57 100644 --- a/app/models/project_services/gemnasium_service.rb +++ b/app/models/project_services/gemnasium_service.rb @@ -12,7 +12,7 @@ def description 'Gemnasium monitors your project dependencies and alerts you about updates and security vulnerabilities.' end - def to_param + def self.to_param 'gemnasium' end @@ -23,10 +23,14 @@ def fields ] end - def supported_events + def self.supported_events %w(push) end + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end + def execute(data) return unless supported_events.include?(data[:object_kind]) diff --git a/app/models/project_services/gitlab_issue_tracker_service.rb b/app/models/project_services/gitlab_issue_tracker_service.rb index 6bd8d4ec5682fd..ad4eb9536e1157 100644 --- a/app/models/project_services/gitlab_issue_tracker_service.rb +++ b/app/models/project_services/gitlab_issue_tracker_service.rb @@ -7,7 +7,7 @@ class GitlabIssueTrackerService < IssueTrackerService default_value_for :default, true - def to_param + def self.to_param 'gitlab' end diff --git a/app/models/project_services/hipchat_service.rb b/app/models/project_services/hipchat_service.rb index 915f6fed74c01c..a675a857b06c12 100644 --- a/app/models/project_services/hipchat_service.rb +++ b/app/models/project_services/hipchat_service.rb @@ -27,7 +27,7 @@ def description 'Private group chat and IM' end - def to_param + def self.to_param 'hipchat' end @@ -45,10 +45,14 @@ def fields ] end - def supported_events + def self.supported_events %w(push issue confidential_issue merge_request note tag_push build) end + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end + def execute(data) return unless supported_events.include?(data[:object_kind]) message = create_message(data) diff --git a/app/models/project_services/irker_service.rb b/app/models/project_services/irker_service.rb index 7355918feab407..9a0cbcbca2d360 100644 --- a/app/models/project_services/irker_service.rb +++ b/app/models/project_services/irker_service.rb @@ -17,14 +17,18 @@ def description 'gateway.' end - def to_param + def self.to_param 'irker' end - def supported_events + def self.supported_events %w(push) end + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end + def execute(data) return unless supported_events.include?(data[:object_kind]) diff --git a/app/models/project_services/issue_tracker_service.rb b/app/models/project_services/issue_tracker_service.rb index bce2cdd55165ee..9e65fdbf9d6618 100644 --- a/app/models/project_services/issue_tracker_service.rb +++ b/app/models/project_services/issue_tracker_service.rb @@ -57,7 +57,7 @@ def initialize_properties(&block) end end - def supported_events + def self.supported_events %w(push) end diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb index 2d969d2fcb66bf..46291f2dcc0d97 100644 --- a/app/models/project_services/jira_service.rb +++ b/app/models/project_services/jira_service.rb @@ -12,10 +12,14 @@ class JiraService < IssueTrackerService # This is confusing, but JiraService does not really support these events. # The values here are required to display correct options in the service # configuration screen. - def supported_events + def self.supported_events %w(commit merge_request) end + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end + # {PROJECT-KEY}-{NUMBER} Examples: JIRA-1, PROJECT-1 def reference_pattern @reference_pattern ||= %r{(?\b([A-Z][A-Z0-9_]+-)\d+)} @@ -81,7 +85,7 @@ def description end end - def to_param + def self.to_param 'jira' end diff --git a/app/models/project_services/kubernetes_service.rb b/app/models/project_services/kubernetes_service.rb index 085125ca9dce64..e4d5f04d1fdacc 100644 --- a/app/models/project_services/kubernetes_service.rb +++ b/app/models/project_services/kubernetes_service.rb @@ -52,7 +52,7 @@ def help 'deployments with `app=$CI_ENVIRONMENT_SLUG`' end - def to_param + def self.to_param 'kubernetes' end @@ -158,6 +158,14 @@ def kubeclient_ssl_options opts end + def self.supported_events + %w() + end + + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end + def kubeclient_auth_options { bearer_token: token } end diff --git a/app/models/project_services/mattermost_service.rb b/app/models/project_services/mattermost_service.rb index ee8a0b5527561b..939dfea0988fd3 100644 --- a/app/models/project_services/mattermost_service.rb +++ b/app/models/project_services/mattermost_service.rb @@ -7,7 +7,7 @@ def description 'Receive event notifications in Mattermost' end - def to_param + def self.to_param 'mattermost' end @@ -38,4 +38,12 @@ def default_fields def default_channel_placeholder "#town-square" end + + def self.supported_events + %w() + end + + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end end diff --git a/app/models/project_services/mattermost_slash_commands_service.rb b/app/models/project_services/mattermost_slash_commands_service.rb index 2cb481182d7073..f25de22e67f18b 100644 --- a/app/models/project_services/mattermost_slash_commands_service.rb +++ b/app/models/project_services/mattermost_slash_commands_service.rb @@ -15,7 +15,7 @@ def description "Perform common operations on GitLab in Mattermost" end - def to_param + def self.to_param 'mattermost_slash_commands' end @@ -48,4 +48,12 @@ def command(params) method: 'P', username: 'GitLab') end + + def self.supported_events + %w() + end + + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end end diff --git a/app/models/project_services/pipelines_email_service.rb b/app/models/project_services/pipelines_email_service.rb index 745f9bd1b43f49..fdfd01192289bc 100644 --- a/app/models/project_services/pipelines_email_service.rb +++ b/app/models/project_services/pipelines_email_service.rb @@ -15,14 +15,18 @@ def description 'Email the pipelines status to a list of recipients.' end - def to_param + def self.to_param 'pipelines_email' end - def supported_events + def self.supported_events %w[pipeline] end + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end + def execute(data, force: false) return unless supported_events.include?(data[:object_kind]) return unless force || should_pipeline_be_notified?(data) diff --git a/app/models/project_services/pivotaltracker_service.rb b/app/models/project_services/pivotaltracker_service.rb index 5301f9fa0ff600..6e19cd8ddd38f5 100644 --- a/app/models/project_services/pivotaltracker_service.rb +++ b/app/models/project_services/pivotaltracker_service.rb @@ -14,7 +14,7 @@ def description 'Project Management Software (Source Commits Endpoint)' end - def to_param + def self.to_param 'pivotaltracker' end @@ -34,10 +34,14 @@ def fields ] end - def supported_events + def self.supported_events %w(push) end + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end + def execute(data) return unless supported_events.include?(data[:object_kind]) return unless allowed_branch?(data[:ref]) diff --git a/app/models/project_services/pushover_service.rb b/app/models/project_services/pushover_service.rb index 3dd878e4c7d773..223054c1d55954 100644 --- a/app/models/project_services/pushover_service.rb +++ b/app/models/project_services/pushover_service.rb @@ -13,7 +13,7 @@ def description 'Pushover makes it easy to get real-time notifications on your Android device, iPhone, iPad, and Desktop.' end - def to_param + def self.to_param 'pushover' end @@ -61,10 +61,14 @@ def fields ] end - def supported_events + def self.supported_events %w(push) end + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end + def execute(data) return unless supported_events.include?(data[:object_kind]) diff --git a/app/models/project_services/redmine_service.rb b/app/models/project_services/redmine_service.rb index f9da273cf0849e..1b835afc67805e 100644 --- a/app/models/project_services/redmine_service.rb +++ b/app/models/project_services/redmine_service.rb @@ -19,7 +19,15 @@ def description end end - def to_param + def self.to_param 'redmine' end + + def self.supported_events + %w() + end + + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end end diff --git a/app/models/project_services/slack_service.rb b/app/models/project_services/slack_service.rb index 76d233a3cca24d..a7a7390ac93a83 100644 --- a/app/models/project_services/slack_service.rb +++ b/app/models/project_services/slack_service.rb @@ -7,7 +7,7 @@ def description 'Receive event notifications in Slack' end - def to_param + def self.to_param 'slack' end @@ -37,4 +37,12 @@ def default_fields def default_channel_placeholder "#general" end + + def self.supported_events + %w() + end + + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end end diff --git a/app/models/project_services/slack_slash_commands_service.rb b/app/models/project_services/slack_slash_commands_service.rb index 5a7cc0fb329799..b66570c2ece369 100644 --- a/app/models/project_services/slack_slash_commands_service.rb +++ b/app/models/project_services/slack_slash_commands_service.rb @@ -9,7 +9,7 @@ def description "Perform common operations on GitLab in Slack" end - def to_param + def self.to_param 'slack_slash_commands' end @@ -25,4 +25,12 @@ def trigger(params) def format(text) Slack::Notifier::LinkFormatter.format(text) if text end + + def self.supported_events + %w() + end + + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end end diff --git a/app/models/project_services/teamcity_service.rb b/app/models/project_services/teamcity_service.rb index 6726082048fa57..b958333ca458e9 100644 --- a/app/models/project_services/teamcity_service.rb +++ b/app/models/project_services/teamcity_service.rb @@ -43,14 +43,18 @@ def help 'requests build, that setting is in the vsc root advanced settings.' end - def to_param + def self.to_param 'teamcity' end - def supported_events + def self.supported_events %w(push) end + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end + def fields [ { type: 'text', name: 'teamcity_url', diff --git a/app/models/service.rb b/app/models/service.rb index 19ef3ba9c2382a..df8d9a850411cc 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -76,6 +76,7 @@ def help def to_param # implement inside child + self.class.to_param end def fields @@ -92,7 +93,8 @@ def event_channel_names end def event_names - supported_events.map { |event| "#{event}_events" } + # implement inside child + self.class.event_names end def event_field(event) @@ -104,7 +106,7 @@ def global_fields end def supported_events - %w(push tag_push issue confidential_issue merge_request wiki_page) + self.class.supported_events end def execute(data) diff --git a/changelogs/unreleased/22974-trigger-service-events-through-api.yml b/changelogs/unreleased/22974-trigger-service-events-through-api.yml new file mode 100644 index 00000000000000..57106e8c676b32 --- /dev/null +++ b/changelogs/unreleased/22974-trigger-service-events-through-api.yml @@ -0,0 +1,4 @@ +--- +title: Adds service trigger events to api +merge_request: 8324 +author: diff --git a/lib/api/services.rb b/lib/api/services.rb index 3a9dfbb237c3e5..907f80db4387ad 100644 --- a/lib/api/services.rb +++ b/lib/api/services.rb @@ -145,7 +145,7 @@ class Services < Grape::API name: :room, type: String, desc: 'Campfire room' - }, + } ], 'custom-issue-tracker' => [ { @@ -534,7 +534,36 @@ class Services < Grape::API desc: 'The password of the user' } ] - }.freeze + } + + service_classes = [ + AsanaService, + AssemblaService, + BambooService, + BugzillaService, + BuildkiteService, + BuildsEmailService, + CampfireService, + CustomIssueTrackerService, + DroneCiService, + EmailsOnPushService, + ExternalWikiService, + FlowdockService, + GemnasiumService, + HipchatService, + IrkerService, + JiraService, + KubernetesService, + MattermostSlashCommandsService, + SlackSlashCommandsService, + PipelinesEmailService, + PivotaltrackerService, + PushoverService, + RedmineService, + SlackService, + MattermostService, + TeamcityService, + ].freeze trigger_services = { 'mattermost-slash-commands' => [ @@ -568,6 +597,20 @@ def service_attributes(service) services.each do |service_slug, settings| desc "Set #{service_slug} service for project" params do + service_classes.each do |service| + event_names = service.try(:event_names) || [] + event_names.each do |event_name| + services[service.to_param.gsub("_", "-")] << { + required: false, + name: event_name.to_sym, + type: String, + desc: ServicesHelper.instance_method(:service_event_description) + .bind(self).call(event_name) + } + end + end + services.freeze + settings.each do |setting| if setting[:required] requires setting[:name], type: setting[:type], desc: setting[:desc] @@ -581,7 +624,7 @@ def service_attributes(service) service_params = declared_params(include_missing: false).merge(active: true) if service.update_attributes(service_params) - true + present service, with: Entities::ProjectService, include_passwords: current_user.is_admin? else render_api_error!('400 Bad Request', 400) end diff --git a/spec/requests/api/services_spec.rb b/spec/requests/api/services_spec.rb index 39c9e0505d1a9c..776dc6556506ba 100644 --- a/spec/requests/api/services_spec.rb +++ b/spec/requests/api/services_spec.rb @@ -6,7 +6,7 @@ let(:user) { create(:user) } let(:admin) { create(:admin) } let(:user2) { create(:user) } - let(:project) {create(:empty_project, creator_id: user.id, namespace: user.namespace) } + let(:project) { create(:empty_project, creator_id: user.id, namespace: user.namespace) } Service.available_services_names.each do |service| describe "PUT /projects/:id/services/#{service.dasherize}" do @@ -16,6 +16,15 @@ put api("/projects/#{project.id}/services/#{dashed_service}", user), service_attrs expect(response).to have_http_status(200) + + current_service = project.services.first + event = current_service.event_names.empty? ? "foo" : current_service.event_names.first + state = current_service[event] || false + + put api("/projects/#{project.id}/services/#{dashed_service}?#{event}=#{!state}", user), service_attrs + + expect(response).to have_http_status(200) + expect(project.services.first[event]).not_to eq(state) unless event == "foo" end it "returns if required fields missing" do -- GitLab From 4b6e583ce0a395ec4a0dbc1a3a81680b1a0aa700 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Thu, 12 Jan 2017 17:33:04 -0500 Subject: [PATCH 24/40] adds test suite --- app/helpers/services_helper.rb | 2 ++ app/models/project_services/asana_service.rb | 4 ---- app/models/project_services/assembla_service.rb | 4 ---- app/models/project_services/bamboo_service.rb | 4 ---- app/models/project_services/bugzilla_service.rb | 8 -------- app/models/project_services/buildkite_service.rb | 8 -------- app/models/project_services/builds_email_service.rb | 4 ---- app/models/project_services/campfire_service.rb | 4 ---- app/models/project_services/ci_service.rb | 3 --- .../custom_issue_tracker_service.rb | 8 -------- .../project_services/emails_on_push_service.rb | 4 ---- .../project_services/external_wiki_service.rb | 4 ---- app/models/project_services/flowdock_service.rb | 4 ---- app/models/project_services/gemnasium_service.rb | 4 ---- app/models/project_services/hipchat_service.rb | 4 ---- app/models/project_services/irker_service.rb | 4 ---- app/models/project_services/jira_service.rb | 4 ---- app/models/project_services/kubernetes_service.rb | 8 -------- app/models/project_services/mattermost_service.rb | 8 -------- .../mattermost_slash_commands_service.rb | 8 -------- .../project_services/pipelines_email_service.rb | 4 ---- .../project_services/pivotaltracker_service.rb | 4 ---- app/models/project_services/pushover_service.rb | 4 ---- app/models/project_services/redmine_service.rb | 8 -------- app/models/project_services/slack_service.rb | 8 -------- .../slack_slash_commands_service.rb | 8 -------- app/models/project_services/teamcity_service.rb | 8 -------- app/models/service.rb | 13 ++++++++++++- lib/api/services.rb | 5 ++--- .../projects/services_controller_spec.rb | 1 + 30 files changed, 17 insertions(+), 147 deletions(-) diff --git a/app/helpers/services_helper.rb b/app/helpers/services_helper.rb index 790ea446e673ef..715e5893a2cd91 100644 --- a/app/helpers/services_helper.rb +++ b/app/helpers/services_helper.rb @@ -26,4 +26,6 @@ def service_event_field_name(event) event = event.pluralize if %w[merge_request issue confidential_issue].include?(event) "#{event}_events" end + + extend self end diff --git a/app/models/project_services/asana_service.rb b/app/models/project_services/asana_service.rb index 20cf7dc4632d55..3728f5642e4322 100644 --- a/app/models/project_services/asana_service.rb +++ b/app/models/project_services/asana_service.rb @@ -48,10 +48,6 @@ def self.supported_events %w(push) end - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end - def client @_client ||= begin Asana::Client.new do |c| diff --git a/app/models/project_services/assembla_service.rb b/app/models/project_services/assembla_service.rb index f18d2204e0e1b3..aeeff8917bf58e 100644 --- a/app/models/project_services/assembla_service.rb +++ b/app/models/project_services/assembla_service.rb @@ -27,10 +27,6 @@ def self.supported_events %w(push) end - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end - def execute(data) return unless supported_events.include?(data[:object_kind]) diff --git a/app/models/project_services/bamboo_service.rb b/app/models/project_services/bamboo_service.rb index fe95fc14525e90..400020ee04aa8e 100644 --- a/app/models/project_services/bamboo_service.rb +++ b/app/models/project_services/bamboo_service.rb @@ -56,10 +56,6 @@ def fields ] end - def self.supported_events - %w(push) - end - def build_page(sha, ref) with_reactive_cache(sha, ref) {|cached| cached[:build_page] } end diff --git a/app/models/project_services/bugzilla_service.rb b/app/models/project_services/bugzilla_service.rb index 18c0cdf2db80eb..046e2809f454ff 100644 --- a/app/models/project_services/bugzilla_service.rb +++ b/app/models/project_services/bugzilla_service.rb @@ -22,12 +22,4 @@ def description def self.to_param 'bugzilla' end - - def self.supported_events - %w() - end - - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end end diff --git a/app/models/project_services/buildkite_service.rb b/app/models/project_services/buildkite_service.rb index 94bc179decaecd..0956c4a4ede143 100644 --- a/app/models/project_services/buildkite_service.rb +++ b/app/models/project_services/buildkite_service.rb @@ -24,14 +24,6 @@ def compose_service_hook hook.save end - def self.supported_events - %w(push) - end - - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end - def execute(data) return unless supported_events.include?(data[:object_kind]) diff --git a/app/models/project_services/builds_email_service.rb b/app/models/project_services/builds_email_service.rb index 51c07f731ffe25..ebd21e3718910b 100644 --- a/app/models/project_services/builds_email_service.rb +++ b/app/models/project_services/builds_email_service.rb @@ -27,10 +27,6 @@ def self.supported_events %w(build) end - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end - def execute(push_data) return unless supported_events.include?(push_data[:object_kind]) return unless should_build_be_notified?(push_data) diff --git a/app/models/project_services/campfire_service.rb b/app/models/project_services/campfire_service.rb index 52371cde20152b..0de59af5652de6 100644 --- a/app/models/project_services/campfire_service.rb +++ b/app/models/project_services/campfire_service.rb @@ -28,10 +28,6 @@ def self.supported_events %w(push) end - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end - def execute(data) return unless supported_events.include?(data[:object_kind]) diff --git a/app/models/project_services/ci_service.rb b/app/models/project_services/ci_service.rb index 9b470c3a07811b..82979c8bd34ed7 100644 --- a/app/models/project_services/ci_service.rb +++ b/app/models/project_services/ci_service.rb @@ -12,9 +12,6 @@ def self.supported_events %w(push) end - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end # Return complete url to build page # # Ex. diff --git a/app/models/project_services/custom_issue_tracker_service.rb b/app/models/project_services/custom_issue_tracker_service.rb index 9c5c86b1cc9318..dea915a4d056c1 100644 --- a/app/models/project_services/custom_issue_tracker_service.rb +++ b/app/models/project_services/custom_issue_tracker_service.rb @@ -36,12 +36,4 @@ def fields { type: 'text', name: 'new_issue_url', placeholder: 'New Issue url' } ] end - - def self.supported_events - %w() - end - - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end end diff --git a/app/models/project_services/emails_on_push_service.rb b/app/models/project_services/emails_on_push_service.rb index 23dc0bc3790f70..f4f913ee0b6051 100644 --- a/app/models/project_services/emails_on_push_service.rb +++ b/app/models/project_services/emails_on_push_service.rb @@ -20,10 +20,6 @@ def self.supported_events %w(push tag_push) end - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end - def execute(push_data) return unless supported_events.include?(push_data[:object_kind]) diff --git a/app/models/project_services/external_wiki_service.rb b/app/models/project_services/external_wiki_service.rb index 961b1c19a33b93..bdf6fa6a5860eb 100644 --- a/app/models/project_services/external_wiki_service.rb +++ b/app/models/project_services/external_wiki_service.rb @@ -33,8 +33,4 @@ def execute(_data) def self.supported_events %w() end - - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end end diff --git a/app/models/project_services/flowdock_service.rb b/app/models/project_services/flowdock_service.rb index 00a15d12efa30f..10a13c3fbdcb57 100644 --- a/app/models/project_services/flowdock_service.rb +++ b/app/models/project_services/flowdock_service.rb @@ -26,10 +26,6 @@ def self.supported_events %w(push) end - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end - def execute(data) return unless supported_events.include?(data[:object_kind]) diff --git a/app/models/project_services/gemnasium_service.rb b/app/models/project_services/gemnasium_service.rb index b70d74e75c1a57..f271e1f1739c91 100644 --- a/app/models/project_services/gemnasium_service.rb +++ b/app/models/project_services/gemnasium_service.rb @@ -27,10 +27,6 @@ def self.supported_events %w(push) end - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end - def execute(data) return unless supported_events.include?(data[:object_kind]) diff --git a/app/models/project_services/hipchat_service.rb b/app/models/project_services/hipchat_service.rb index a675a857b06c12..72da219df28bf2 100644 --- a/app/models/project_services/hipchat_service.rb +++ b/app/models/project_services/hipchat_service.rb @@ -49,10 +49,6 @@ def self.supported_events %w(push issue confidential_issue merge_request note tag_push build) end - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end - def execute(data) return unless supported_events.include?(data[:object_kind]) message = create_message(data) diff --git a/app/models/project_services/irker_service.rb b/app/models/project_services/irker_service.rb index 9a0cbcbca2d360..5d93064f9b311a 100644 --- a/app/models/project_services/irker_service.rb +++ b/app/models/project_services/irker_service.rb @@ -25,10 +25,6 @@ def self.supported_events %w(push) end - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end - def execute(data) return unless supported_events.include?(data[:object_kind]) diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb index 46291f2dcc0d97..2ac76e97de00d0 100644 --- a/app/models/project_services/jira_service.rb +++ b/app/models/project_services/jira_service.rb @@ -16,10 +16,6 @@ def self.supported_events %w(commit merge_request) end - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end - # {PROJECT-KEY}-{NUMBER} Examples: JIRA-1, PROJECT-1 def reference_pattern @reference_pattern ||= %r{(?\b([A-Z][A-Z0-9_]+-)\d+)} diff --git a/app/models/project_services/kubernetes_service.rb b/app/models/project_services/kubernetes_service.rb index e4d5f04d1fdacc..fa3cedc4354bbd 100644 --- a/app/models/project_services/kubernetes_service.rb +++ b/app/models/project_services/kubernetes_service.rb @@ -158,14 +158,6 @@ def kubeclient_ssl_options opts end - def self.supported_events - %w() - end - - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end - def kubeclient_auth_options { bearer_token: token } end diff --git a/app/models/project_services/mattermost_service.rb b/app/models/project_services/mattermost_service.rb index 939dfea0988fd3..c5c551b3ef58cb 100644 --- a/app/models/project_services/mattermost_service.rb +++ b/app/models/project_services/mattermost_service.rb @@ -38,12 +38,4 @@ def default_fields def default_channel_placeholder "#town-square" end - - def self.supported_events - %w() - end - - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end end diff --git a/app/models/project_services/mattermost_slash_commands_service.rb b/app/models/project_services/mattermost_slash_commands_service.rb index f25de22e67f18b..50a011db74ed17 100644 --- a/app/models/project_services/mattermost_slash_commands_service.rb +++ b/app/models/project_services/mattermost_slash_commands_service.rb @@ -48,12 +48,4 @@ def command(params) method: 'P', username: 'GitLab') end - - def self.supported_events - %w() - end - - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end end diff --git a/app/models/project_services/pipelines_email_service.rb b/app/models/project_services/pipelines_email_service.rb index fdfd01192289bc..ac617f409d9417 100644 --- a/app/models/project_services/pipelines_email_service.rb +++ b/app/models/project_services/pipelines_email_service.rb @@ -23,10 +23,6 @@ def self.supported_events %w[pipeline] end - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end - def execute(data, force: false) return unless supported_events.include?(data[:object_kind]) return unless force || should_pipeline_be_notified?(data) diff --git a/app/models/project_services/pivotaltracker_service.rb b/app/models/project_services/pivotaltracker_service.rb index 6e19cd8ddd38f5..9cc642591f4364 100644 --- a/app/models/project_services/pivotaltracker_service.rb +++ b/app/models/project_services/pivotaltracker_service.rb @@ -38,10 +38,6 @@ def self.supported_events %w(push) end - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end - def execute(data) return unless supported_events.include?(data[:object_kind]) return unless allowed_branch?(data[:ref]) diff --git a/app/models/project_services/pushover_service.rb b/app/models/project_services/pushover_service.rb index 223054c1d55954..a963d27a37652e 100644 --- a/app/models/project_services/pushover_service.rb +++ b/app/models/project_services/pushover_service.rb @@ -65,10 +65,6 @@ def self.supported_events %w(push) end - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end - def execute(data) return unless supported_events.include?(data[:object_kind]) diff --git a/app/models/project_services/redmine_service.rb b/app/models/project_services/redmine_service.rb index 1b835afc67805e..6acf611eba581c 100644 --- a/app/models/project_services/redmine_service.rb +++ b/app/models/project_services/redmine_service.rb @@ -22,12 +22,4 @@ def description def self.to_param 'redmine' end - - def self.supported_events - %w() - end - - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end end diff --git a/app/models/project_services/slack_service.rb b/app/models/project_services/slack_service.rb index a7a7390ac93a83..f77d2d7c60ba92 100644 --- a/app/models/project_services/slack_service.rb +++ b/app/models/project_services/slack_service.rb @@ -37,12 +37,4 @@ def default_fields def default_channel_placeholder "#general" end - - def self.supported_events - %w() - end - - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end end diff --git a/app/models/project_services/slack_slash_commands_service.rb b/app/models/project_services/slack_slash_commands_service.rb index b66570c2ece369..c34991e426287d 100644 --- a/app/models/project_services/slack_slash_commands_service.rb +++ b/app/models/project_services/slack_slash_commands_service.rb @@ -25,12 +25,4 @@ def trigger(params) def format(text) Slack::Notifier::LinkFormatter.format(text) if text end - - def self.supported_events - %w() - end - - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end end diff --git a/app/models/project_services/teamcity_service.rb b/app/models/project_services/teamcity_service.rb index b958333ca458e9..cbaffb8ce48558 100644 --- a/app/models/project_services/teamcity_service.rb +++ b/app/models/project_services/teamcity_service.rb @@ -47,14 +47,6 @@ def self.to_param 'teamcity' end - def self.supported_events - %w(push) - end - - def self.event_names - self.supported_events.map { |event| "#{event}_events" } - end - def fields [ { type: 'text', name: 'teamcity_url', diff --git a/app/models/service.rb b/app/models/service.rb index df8d9a850411cc..043be222f3a401 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -79,6 +79,10 @@ def to_param self.class.to_param end + def self.to_param + raise NotImplementedError + end + def fields # implement inside child [] @@ -93,10 +97,13 @@ def event_channel_names end def event_names - # implement inside child self.class.event_names end + def self.event_names + self.supported_events.map { |event| "#{event}_events" } + end + def event_field(event) nil end @@ -109,6 +116,10 @@ def supported_events self.class.supported_events end + def self.supported_events + %w(push tag_push issue confidential_issue merge_request wiki_page) + end + def execute(data) # implement inside child end diff --git a/lib/api/services.rb b/lib/api/services.rb index 907f80db4387ad..a0abec494385c3 100644 --- a/lib/api/services.rb +++ b/lib/api/services.rb @@ -600,12 +600,11 @@ def service_attributes(service) service_classes.each do |service| event_names = service.try(:event_names) || [] event_names.each do |event_name| - services[service.to_param.gsub("_", "-")] << { + services[service.to_param.tr("_", "-")] << { required: false, name: event_name.to_sym, type: String, - desc: ServicesHelper.instance_method(:service_event_description) - .bind(self).call(event_name) + desc: ServicesHelper.service_event_description(event_name) } end end diff --git a/spec/controllers/projects/services_controller_spec.rb b/spec/controllers/projects/services_controller_spec.rb index 2e44b5128b421d..a6e708c01e4a08 100644 --- a/spec/controllers/projects/services_controller_spec.rb +++ b/spec/controllers/projects/services_controller_spec.rb @@ -54,6 +54,7 @@ context 'on successful update' do it 'sets the flash' do expect(service).to receive(:to_param).and_return('hipchat') + expect(service).to receive(:event_names).and_return(HipchatService.event_names) put :update, namespace_id: project.namespace.id, -- GitLab From 17f0c99c39f7919f7b8ae97034aa0d1cf646017f Mon Sep 17 00:00:00 2001 From: Mike Greiling Date: Sat, 21 Jan 2017 17:23:07 -0600 Subject: [PATCH 25/40] remove vestigial onsubmit event trigger --- app/views/shared/issuable/_search_bar.html.haml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/views/shared/issuable/_search_bar.html.haml b/app/views/shared/issuable/_search_bar.html.haml index 44152319736bcd..be5f08d8a40e9c 100644 --- a/app/views/shared/issuable/_search_bar.html.haml +++ b/app/views/shared/issuable/_search_bar.html.haml @@ -125,10 +125,6 @@ new MilestoneSelect(); new IssueStatusSelect(); new SubscriptionSelect(); - $('form.filter-form').on('submit', function (event) { - event.preventDefault(); - Turbolinks.visit(this.action + '&' + $(this).serialize()); - }); $(document).off('page:restore').on('page:restore', function (event) { if (gl.FilteredSearchManager) { -- GitLab From 37382d3299c7d229931295fe9848674b3121ae7d Mon Sep 17 00:00:00 2001 From: Mike Greiling Date: Sat, 21 Jan 2017 01:44:36 -0600 Subject: [PATCH 26/40] allow issue filter submission by clicking "Keep typing and press enter" --- .../javascripts/filtered_search/dropdown_hint.js.es6 | 3 +++ .../filtered_search/filtered_search_dropdown.js.es6 | 6 ++++++ .../filtered_search/filtered_search_manager.js.es6 | 8 ++++++++ app/views/shared/issuable/_search_bar.html.haml | 2 +- 4 files changed, 18 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/filtered_search/dropdown_hint.js.es6 b/app/assets/javascripts/filtered_search/dropdown_hint.js.es6 index f4ec3b206ccfd0..7d297b8eee8666 100644 --- a/app/assets/javascripts/filtered_search/dropdown_hint.js.es6 +++ b/app/assets/javascripts/filtered_search/dropdown_hint.js.es6 @@ -20,6 +20,9 @@ if (selected.tagName === 'LI') { if (selected.hasAttribute('data-value')) { this.dismissDropdown(); + } else if (selected.getAttribute('data-action') === 'submit') { + this.dismissDropdown(); + this.dispatchFormSubmitEvent(); } else { const token = selected.querySelector('.js-filter-hint').innerText.trim(); const tag = selected.querySelector('.js-filter-tag').innerText.trim(); diff --git a/app/assets/javascripts/filtered_search/filtered_search_dropdown.js.es6 b/app/assets/javascripts/filtered_search/filtered_search_dropdown.js.es6 index 9128ea907b3ce6..d2e7b61b345e9f 100644 --- a/app/assets/javascripts/filtered_search/filtered_search_dropdown.js.es6 +++ b/app/assets/javascripts/filtered_search/filtered_search_dropdown.js.es6 @@ -84,6 +84,12 @@ })); } + dispatchFormSubmitEvent() { + // dispatchEvent() is necessary as form.submit() does not + // trigger event handlers + this.input.form.dispatchEvent(new Event('submit')); + } + hideDropdown() { this.getCurrentHook().list.hide(); } diff --git a/app/assets/javascripts/filtered_search/filtered_search_manager.js.es6 b/app/assets/javascripts/filtered_search/filtered_search_manager.js.es6 index c7b72b365615b3..ae19bb68360049 100644 --- a/app/assets/javascripts/filtered_search/filtered_search_manager.js.es6 +++ b/app/assets/javascripts/filtered_search/filtered_search_manager.js.es6 @@ -25,6 +25,7 @@ } bindEvents() { + this.handleFormSubmit = this.handleFormSubmit.bind(this); this.setDropdownWrapper = this.dropdownManager.setDropdown.bind(this.dropdownManager); this.toggleClearSearchButtonWrapper = this.toggleClearSearchButton.bind(this); this.checkForEnterWrapper = this.checkForEnter.bind(this); @@ -32,6 +33,7 @@ this.checkForBackspaceWrapper = this.checkForBackspace.bind(this); this.tokenChange = this.tokenChange.bind(this); + this.filteredSearchInput.form.addEventListener('submit', this.handleFormSubmit); this.filteredSearchInput.addEventListener('input', this.setDropdownWrapper); this.filteredSearchInput.addEventListener('input', this.toggleClearSearchButtonWrapper); this.filteredSearchInput.addEventListener('keydown', this.checkForEnterWrapper); @@ -42,6 +44,7 @@ } unbindEvents() { + this.filteredSearchInput.form.removeEventListener('submit', this.handleFormSubmit); this.filteredSearchInput.removeEventListener('input', this.setDropdownWrapper); this.filteredSearchInput.removeEventListener('input', this.toggleClearSearchButtonWrapper); this.filteredSearchInput.removeEventListener('keydown', this.checkForEnterWrapper); @@ -88,6 +91,11 @@ this.dropdownManager.resetDropdowns(); } + handleFormSubmit(e) { + e.preventDefault(); + this.search(); + } + loadSearchParamsFromURL() { const params = gl.utils.getUrlParamsArray(); const usernameParams = this.getUsernameParams(); diff --git a/app/views/shared/issuable/_search_bar.html.haml b/app/views/shared/issuable/_search_bar.html.haml index be5f08d8a40e9c..e9644ca0f12369 100644 --- a/app/views/shared/issuable/_search_bar.html.haml +++ b/app/views/shared/issuable/_search_bar.html.haml @@ -17,7 +17,7 @@ = icon('times') #js-dropdown-hint.dropdown-menu.hint-dropdown %ul{ 'data-dropdown' => true } - %li.filter-dropdown-item{ 'data-value' => '' } + %li.filter-dropdown-item{ 'data-action' => 'submit' } %button.btn.btn-link = icon('search') %span -- GitLab From d07acd42b5bcf6d4ec55bcfa7f99932319c522b3 Mon Sep 17 00:00:00 2001 From: Mike Greiling Date: Sat, 21 Jan 2017 13:11:28 -0600 Subject: [PATCH 27/40] add a space after selecting a dropdown item --- .../filtered_search/filtered_search_dropdown.js.es6 | 1 + .../filtered_search_dropdown_manager.js.es6 | 12 ++++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/filtered_search/filtered_search_dropdown.js.es6 b/app/assets/javascripts/filtered_search/filtered_search_dropdown.js.es6 index d2e7b61b345e9f..859d6515531e78 100644 --- a/app/assets/javascripts/filtered_search/filtered_search_dropdown.js.es6 +++ b/app/assets/javascripts/filtered_search/filtered_search_dropdown.js.es6 @@ -39,6 +39,7 @@ } this.dismissDropdown(); + this.dispatchInputEvent(); } } diff --git a/app/assets/javascripts/filtered_search/filtered_search_dropdown_manager.js.es6 b/app/assets/javascripts/filtered_search/filtered_search_dropdown_manager.js.es6 index 408a0dfd76892f..00e1c28692f3f2 100644 --- a/app/assets/javascripts/filtered_search/filtered_search_dropdown_manager.js.es6 +++ b/app/assets/javascripts/filtered_search/filtered_search_dropdown_manager.js.es6 @@ -61,11 +61,19 @@ const word = `${tokenName}:${tokenValue}`; // Get the string to replace - const selectionStart = input.selectionStart; + let newCaretPosition = input.selectionStart; const { left, right } = gl.DropdownUtils.getInputSelectionPosition(input); input.value = `${inputValue.substr(0, left)}${word}${inputValue.substr(right)}`; - gl.FilteredSearchDropdownManager.updateInputCaretPosition(selectionStart, input); + + // If we have added a tokenValue at the end of the input, + // add a space and set selection to the end + if (right >= inputValue.length && tokenValue !== '') { + input.value += ' '; + newCaretPosition = input.value.length; + } + + gl.FilteredSearchDropdownManager.updateInputCaretPosition(newCaretPosition, input); } static updateInputCaretPosition(selectionStart, input) { -- GitLab From deefcb19512da422cb33f1032b05fd6876cc57c0 Mon Sep 17 00:00:00 2001 From: Mike Greiling Date: Sat, 21 Jan 2017 23:28:37 -0600 Subject: [PATCH 28/40] update tests to correspond with new behavior --- .../filtered_search/dropdown_assignee_spec.rb | 8 ++++---- .../filtered_search/dropdown_author_spec.rb | 4 ++-- .../filtered_search/dropdown_label_spec.rb | 16 ++++++++-------- .../filtered_search/dropdown_milestone_spec.rb | 18 +++++++++--------- .../filtered_search/filter_issues_spec.rb | 6 +++--- ...iltered_search_dropdown_manager_spec.js.es6 | 6 +++--- 6 files changed, 29 insertions(+), 29 deletions(-) diff --git a/spec/features/issues/filtered_search/dropdown_assignee_spec.rb b/spec/features/issues/filtered_search/dropdown_assignee_spec.rb index 16dcc487812e4f..8a155c3bfc5dbc 100644 --- a/spec/features/issues/filtered_search/dropdown_assignee_spec.rb +++ b/spec/features/issues/filtered_search/dropdown_assignee_spec.rb @@ -134,14 +134,14 @@ def click_assignee(text) click_button 'Assigned to me' end - expect(filtered_search.value).to eq("assignee:#{user.to_reference}") + expect(filtered_search.value).to eq("assignee:#{user.to_reference} ") end it 'fills in the assignee username when the assignee has not been filtered' do click_assignee(user_jacob.name) expect(page).to have_css(js_dropdown_assignee, visible: false) - expect(filtered_search.value).to eq("assignee:@#{user_jacob.username}") + expect(filtered_search.value).to eq("assignee:@#{user_jacob.username} ") end it 'fills in the assignee username when the assignee has been filtered' do @@ -149,14 +149,14 @@ def click_assignee(text) click_assignee(user.name) expect(page).to have_css(js_dropdown_assignee, visible: false) - expect(filtered_search.value).to eq("assignee:@#{user.username}") + expect(filtered_search.value).to eq("assignee:@#{user.username} ") end it 'selects `no assignee`' do find('#js-dropdown-assignee .filter-dropdown-item', text: 'No Assignee').click expect(page).to have_css(js_dropdown_assignee, visible: false) - expect(filtered_search.value).to eq("assignee:none") + expect(filtered_search.value).to eq("assignee:none ") end end diff --git a/spec/features/issues/filtered_search/dropdown_author_spec.rb b/spec/features/issues/filtered_search/dropdown_author_spec.rb index 464749d01e307f..a5d5d9d4c5e025 100644 --- a/spec/features/issues/filtered_search/dropdown_author_spec.rb +++ b/spec/features/issues/filtered_search/dropdown_author_spec.rb @@ -121,14 +121,14 @@ def click_author(text) click_author(user_jacob.name) expect(page).to have_css(js_dropdown_author, visible: false) - expect(filtered_search.value).to eq("author:@#{user_jacob.username}") + expect(filtered_search.value).to eq("author:@#{user_jacob.username} ") end it 'fills in the author username when the author has been filtered' do click_author(user.name) expect(page).to have_css(js_dropdown_author, visible: false) - expect(filtered_search.value).to eq("author:@#{user.username}") + expect(filtered_search.value).to eq("author:@#{user.username} ") end end diff --git a/spec/features/issues/filtered_search/dropdown_label_spec.rb b/spec/features/issues/filtered_search/dropdown_label_spec.rb index 89c144141c9366..bea00160f96413 100644 --- a/spec/features/issues/filtered_search/dropdown_label_spec.rb +++ b/spec/features/issues/filtered_search/dropdown_label_spec.rb @@ -159,7 +159,7 @@ def click_label(text) click_label(bug_label.title) expect(page).to have_css(js_dropdown_label, visible: false) - expect(filtered_search.value).to eq("label:~#{bug_label.title}") + expect(filtered_search.value).to eq("label:~#{bug_label.title} ") end it 'fills in the label name when the label is partially filled' do @@ -167,49 +167,49 @@ def click_label(text) click_label(bug_label.title) expect(page).to have_css(js_dropdown_label, visible: false) - expect(filtered_search.value).to eq("label:~#{bug_label.title}") + expect(filtered_search.value).to eq("label:~#{bug_label.title} ") end it 'fills in the label name that contains multiple words' do click_label(two_words_label.title) expect(page).to have_css(js_dropdown_label, visible: false) - expect(filtered_search.value).to eq("label:~\"#{two_words_label.title}\"") + expect(filtered_search.value).to eq("label:~\"#{two_words_label.title}\" ") end it 'fills in the label name that contains multiple words and is very long' do click_label(long_label.title) expect(page).to have_css(js_dropdown_label, visible: false) - expect(filtered_search.value).to eq("label:~\"#{long_label.title}\"") + expect(filtered_search.value).to eq("label:~\"#{long_label.title}\" ") end it 'fills in the label name that contains double quotes' do click_label(wont_fix_label.title) expect(page).to have_css(js_dropdown_label, visible: false) - expect(filtered_search.value).to eq("label:~'#{wont_fix_label.title}'") + expect(filtered_search.value).to eq("label:~'#{wont_fix_label.title}' ") end it 'fills in the label name with the correct capitalization' do click_label(uppercase_label.title) expect(page).to have_css(js_dropdown_label, visible: false) - expect(filtered_search.value).to eq("label:~#{uppercase_label.title}") + expect(filtered_search.value).to eq("label:~#{uppercase_label.title} ") end it 'fills in the label name with special characters' do click_label(special_label.title) expect(page).to have_css(js_dropdown_label, visible: false) - expect(filtered_search.value).to eq("label:~#{special_label.title}") + expect(filtered_search.value).to eq("label:~#{special_label.title} ") end it 'selects `no label`' do find('#js-dropdown-label .filter-dropdown-item', text: 'No Label').click expect(page).to have_css(js_dropdown_label, visible: false) - expect(filtered_search.value).to eq("label:none") + expect(filtered_search.value).to eq("label:none ") end end diff --git a/spec/features/issues/filtered_search/dropdown_milestone_spec.rb b/spec/features/issues/filtered_search/dropdown_milestone_spec.rb index e5a271b663f186..134e58ad586e1c 100644 --- a/spec/features/issues/filtered_search/dropdown_milestone_spec.rb +++ b/spec/features/issues/filtered_search/dropdown_milestone_spec.rb @@ -127,7 +127,7 @@ def click_static_milestone(text) click_milestone(milestone.title) expect(page).to have_css(js_dropdown_milestone, visible: false) - expect(filtered_search.value).to eq("milestone:%#{milestone.title}") + expect(filtered_search.value).to eq("milestone:%#{milestone.title} ") end it 'fills in the milestone name when the milestone is partially filled' do @@ -135,56 +135,56 @@ def click_static_milestone(text) click_milestone(milestone.title) expect(page).to have_css(js_dropdown_milestone, visible: false) - expect(filtered_search.value).to eq("milestone:%#{milestone.title}") + expect(filtered_search.value).to eq("milestone:%#{milestone.title} ") end it 'fills in the milestone name that contains multiple words' do click_milestone(two_words_milestone.title) expect(page).to have_css(js_dropdown_milestone, visible: false) - expect(filtered_search.value).to eq("milestone:%\"#{two_words_milestone.title}\"") + expect(filtered_search.value).to eq("milestone:%\"#{two_words_milestone.title}\" ") end it 'fills in the milestone name that contains multiple words and is very long' do click_milestone(long_milestone.title) expect(page).to have_css(js_dropdown_milestone, visible: false) - expect(filtered_search.value).to eq("milestone:%\"#{long_milestone.title}\"") + expect(filtered_search.value).to eq("milestone:%\"#{long_milestone.title}\" ") end it 'fills in the milestone name that contains double quotes' do click_milestone(wont_fix_milestone.title) expect(page).to have_css(js_dropdown_milestone, visible: false) - expect(filtered_search.value).to eq("milestone:%'#{wont_fix_milestone.title}'") + expect(filtered_search.value).to eq("milestone:%'#{wont_fix_milestone.title}' ") end it 'fills in the milestone name with the correct capitalization' do click_milestone(uppercase_milestone.title) expect(page).to have_css(js_dropdown_milestone, visible: false) - expect(filtered_search.value).to eq("milestone:%#{uppercase_milestone.title}") + expect(filtered_search.value).to eq("milestone:%#{uppercase_milestone.title} ") end it 'fills in the milestone name with special characters' do click_milestone(special_milestone.title) expect(page).to have_css(js_dropdown_milestone, visible: false) - expect(filtered_search.value).to eq("milestone:%#{special_milestone.title}") + expect(filtered_search.value).to eq("milestone:%#{special_milestone.title} ") end it 'selects `no milestone`' do click_static_milestone('No Milestone') expect(page).to have_css(js_dropdown_milestone, visible: false) - expect(filtered_search.value).to eq("milestone:none") + expect(filtered_search.value).to eq("milestone:none ") end it 'selects `upcoming milestone`' do click_static_milestone('Upcoming') expect(page).to have_css(js_dropdown_milestone, visible: false) - expect(filtered_search.value).to eq("milestone:upcoming") + expect(filtered_search.value).to eq("milestone:upcoming ") end end diff --git a/spec/features/issues/filtered_search/filter_issues_spec.rb b/spec/features/issues/filtered_search/filter_issues_spec.rb index 1cdac520181c11..f48a0193545511 100644 --- a/spec/features/issues/filtered_search/filter_issues_spec.rb +++ b/spec/features/issues/filtered_search/filter_issues_spec.rb @@ -539,7 +539,7 @@ def select_search_at_index(pos) click_button user2.username end - expect(filtered_search.value).to eq("author:@#{user2.username}") + expect(filtered_search.value).to eq("author:@#{user2.username} ") end it 'changes label' do @@ -551,7 +551,7 @@ def select_search_at_index(pos) click_button label.name end - expect(filtered_search.value).to eq("author:@#{user.username} label:~#{label.name}") + expect(filtered_search.value).to eq("author:@#{user.username} label:~#{label.name} ") end it 'changes label correctly space is in previous label' do @@ -563,7 +563,7 @@ def select_search_at_index(pos) click_button label.name end - expect(filtered_search.value).to eq("label:~#{label.name}") + expect(filtered_search.value).to eq("label:~#{label.name} ") end end diff --git a/spec/javascripts/filtered_search/filtered_search_dropdown_manager_spec.js.es6 b/spec/javascripts/filtered_search/filtered_search_dropdown_manager_spec.js.es6 index d0d27ceb4a6f8b..4bd45eb457d075 100644 --- a/spec/javascripts/filtered_search/filtered_search_dropdown_manager_spec.js.es6 +++ b/spec/javascripts/filtered_search/filtered_search_dropdown_manager_spec.js.es6 @@ -31,7 +31,7 @@ it('should add tokenName and tokenValue', () => { gl.FilteredSearchDropdownManager.addWordToInput('label', 'none'); - expect(getInputValue()).toBe('label:none'); + expect(getInputValue()).toBe('label:none '); }); }); @@ -45,13 +45,13 @@ it('should replace tokenValue', () => { setInputValue('author:roo'); gl.FilteredSearchDropdownManager.addWordToInput('author', '@root'); - expect(getInputValue()).toBe('author:@root'); + expect(getInputValue()).toBe('author:@root '); }); it('should add tokenValues containing spaces', () => { setInputValue('label:~"test'); gl.FilteredSearchDropdownManager.addWordToInput('label', '~\'"test me"\''); - expect(getInputValue()).toBe('label:~\'"test me"\''); + expect(getInputValue()).toBe('label:~\'"test me"\' '); }); }); }); -- GitLab From b994596b5a27fa8959502cc4f96eacbf086e328c Mon Sep 17 00:00:00 2001 From: Mike Greiling Date: Sat, 21 Jan 2017 23:29:35 -0600 Subject: [PATCH 29/40] add CHANGELOG.md entry for !8681 --- changelogs/unreleased/issue-filter-click-to-search.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/issue-filter-click-to-search.yml diff --git a/changelogs/unreleased/issue-filter-click-to-search.yml b/changelogs/unreleased/issue-filter-click-to-search.yml new file mode 100644 index 00000000000000..c024ea48dc7183 --- /dev/null +++ b/changelogs/unreleased/issue-filter-click-to-search.yml @@ -0,0 +1,4 @@ +--- +title: allow issue filter bar to be operated with mouse only +merge_request: 8681 +author: -- GitLab From a92c0f9c116a2d16106a8be4e802fc7dbb826e2f Mon Sep 17 00:00:00 2001 From: Semyon Pupkov Date: Thu, 19 Jan 2017 11:35:50 +0500 Subject: [PATCH 30/40] Use archived trait for project in specs instead model column --- spec/features/admin/admin_projects_spec.rb | 2 +- spec/features/groups/merge_requests_spec.rb | 2 +- spec/finders/merge_requests_finder_spec.rb | 2 +- spec/finders/move_to_project_finder_spec.rb | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/features/admin/admin_projects_spec.rb b/spec/features/admin/admin_projects_spec.rb index a5b88812b75594..87a8f62687aa91 100644 --- a/spec/features/admin/admin_projects_spec.rb +++ b/spec/features/admin/admin_projects_spec.rb @@ -10,7 +10,7 @@ end describe "GET /admin/projects" do - let!(:archived_project) { create :project, :public, archived: true } + let!(:archived_project) { create :project, :public, :archived } before do visit admin_projects_path diff --git a/spec/features/groups/merge_requests_spec.rb b/spec/features/groups/merge_requests_spec.rb index 30b80aa82b02fe..78a11ffee99d46 100644 --- a/spec/features/groups/merge_requests_spec.rb +++ b/spec/features/groups/merge_requests_spec.rb @@ -7,7 +7,7 @@ include_examples 'project features apply to issuables', MergeRequest context 'archived issuable' do - let(:project_archived) { create(:project, group: group, merge_requests_access_level: ProjectFeature::ENABLED, archived: true) } + let(:project_archived) { create(:project, :archived, group: group, merge_requests_access_level: ProjectFeature::ENABLED) } let(:issuable_archived) { create(:merge_request, source_project: project_archived, target_project: project_archived, title: 'issuable of an archived project') } let(:access_level) { ProjectFeature::ENABLED } let(:user) { user_in_group } diff --git a/spec/finders/merge_requests_finder_spec.rb b/spec/finders/merge_requests_finder_spec.rb index 88361e27102e32..e4ba1d2f1c2bb5 100644 --- a/spec/finders/merge_requests_finder_spec.rb +++ b/spec/finders/merge_requests_finder_spec.rb @@ -6,7 +6,7 @@ let(:project1) { create(:project) } let(:project2) { create(:project, forked_from_project: project1) } - let(:project3) { create(:project, forked_from_project: project1, archived: true) } + let(:project3) { create(:project, :archived, forked_from_project: project1) } let!(:merge_request1) { create(:merge_request, :simple, author: user, source_project: project2, target_project: project1) } let!(:merge_request2) { create(:merge_request, :simple, author: user, source_project: project2, target_project: project1, state: 'closed') } diff --git a/spec/finders/move_to_project_finder_spec.rb b/spec/finders/move_to_project_finder_spec.rb index 8488dbd2a1634b..dea87980e25f84 100644 --- a/spec/finders/move_to_project_finder_spec.rb +++ b/spec/finders/move_to_project_finder_spec.rb @@ -36,7 +36,7 @@ it 'does not return archived projects' do reporter_project.team << [user, :reporter] - reporter_project.update_attributes(archived: true) + reporter_project.archive! other_reporter_project = create(:empty_project) other_reporter_project.team << [user, :reporter] -- GitLab From 03206a87c730b51ef8506badc510ab37ba36cf51 Mon Sep 17 00:00:00 2001 From: matmen Date: Sun, 22 Jan 2017 17:35:11 +0000 Subject: [PATCH 31/40] Fix the most important part of the readme Change the twitter `/favorites` to `/likes` --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 4e28f3aacfd69a..4f85fac4a569a2 100644 --- a/README.md +++ b/README.md @@ -113,4 +113,4 @@ Please see [Getting help for GitLab](https://about.gitlab.com/getting-help/) on ## Is it awesome? Thanks for [asking this question](https://twitter.com/supersloth/status/489462789384056832) Joshua. -[These people](https://twitter.com/gitlab/favorites) seem to like it. +[These people](https://twitter.com/gitlab/likes) seem to like it. -- GitLab From fca41b41e8e387b3a672679b155dfcd27422a26c Mon Sep 17 00:00:00 2001 From: Ryan Harris Date: Tue, 17 Jan 2017 20:20:35 -0500 Subject: [PATCH 32/40] Add hover style for copy icon in commit page header Change hover style definition from explicit rgba value to variable Removed style from page-header.scss that was overriding .btn styles Add hover style for copy icon in commit page header Change hover style definition from explicit rgba value to variable Removed unnecessary styles on commits.scss --- app/assets/stylesheets/framework/page-header.scss | 4 ---- changelogs/unreleased/26787-add-copy-icon-hover-state.yml | 4 ++++ 2 files changed, 4 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/26787-add-copy-icon-hover-state.yml diff --git a/app/assets/stylesheets/framework/page-header.scss b/app/assets/stylesheets/framework/page-header.scss index 4decee2c525c5a..5f4211147f30fd 100644 --- a/app/assets/stylesheets/framework/page-header.scss +++ b/app/assets/stylesheets/framework/page-header.scss @@ -46,10 +46,6 @@ font-weight: bold; } - .fa-clipboard { - color: $dropdown-title-btn-color; - } - .commit-info { &.branches { margin-left: 8px; diff --git a/changelogs/unreleased/26787-add-copy-icon-hover-state.yml b/changelogs/unreleased/26787-add-copy-icon-hover-state.yml new file mode 100644 index 00000000000000..31f1812c6f805d --- /dev/null +++ b/changelogs/unreleased/26787-add-copy-icon-hover-state.yml @@ -0,0 +1,4 @@ +--- +title: Add hover style to copy icon on commit page header +merge_request: +author: Ryan Harris -- GitLab From f287ae24c0387315aff61658bc1baa03317bd465 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Mon, 23 Jan 2017 10:32:30 -0600 Subject: [PATCH 33/40] Remove hover styling for generic textarea --- app/assets/stylesheets/pages/search.scss | 1 - changelogs/unreleased/27066-textarea-border.yml | 4 ++++ 2 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/27066-textarea-border.yml diff --git a/app/assets/stylesheets/pages/search.scss b/app/assets/stylesheets/pages/search.scss index 12bff32bbf3864..88ea92c5afb1e7 100644 --- a/app/assets/stylesheets/pages/search.scss +++ b/app/assets/stylesheets/pages/search.scss @@ -18,7 +18,6 @@ .file-finder-input:hover, .issuable-search-form:hover, .search-text-input:hover, -textarea:hover, .form-control:hover { border-color: lighten($dropdown-input-focus-border, 20%); box-shadow: 0 0 4px lighten($search-input-focus-shadow-color, 20%); diff --git a/changelogs/unreleased/27066-textarea-border.yml b/changelogs/unreleased/27066-textarea-border.yml new file mode 100644 index 00000000000000..e45cb3aced5279 --- /dev/null +++ b/changelogs/unreleased/27066-textarea-border.yml @@ -0,0 +1,4 @@ +--- +title: Remove blue border from comment box hover +merge_request: +author: -- GitLab From 60d1dcb83ac97e3d0dfd9cdf0daa970671ba3d68 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 19 Jan 2017 17:11:48 +0000 Subject: [PATCH 34/40] Merge branch 'fix-users-deleting-public-deployment-keys' into 'security' Fix users being able to delete instance public deployment keys See merge request !2049 --- .../fix-users-deleting-public-deployment-keys.yml | 4 ++++ lib/api/deploy_keys.rb | 10 +++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/fix-users-deleting-public-deployment-keys.yml diff --git a/changelogs/unreleased/fix-users-deleting-public-deployment-keys.yml b/changelogs/unreleased/fix-users-deleting-public-deployment-keys.yml new file mode 100644 index 00000000000000..c9edd1de86c89f --- /dev/null +++ b/changelogs/unreleased/fix-users-deleting-public-deployment-keys.yml @@ -0,0 +1,4 @@ +--- +title: Prevent users from deleting system deploy keys via the project deploy key API +merge_request: +author: diff --git a/lib/api/deploy_keys.rb b/lib/api/deploy_keys.rb index 853607308412a8..f6cb17bafd819f 100644 --- a/lib/api/deploy_keys.rb +++ b/lib/api/deploy_keys.rb @@ -105,15 +105,19 @@ class DeployKeys < Grape::API present key.deploy_key, with: Entities::SSHKey end - desc 'Delete existing deploy key of currently authenticated user' do + desc 'Delete deploy key for a project' do success Key end params do requires :key_id, type: Integer, desc: 'The ID of the deploy key' end delete ":id/#{path}/:key_id" do - key = user_project.deploy_keys.find(params[:key_id]) - key.destroy + key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id]) + if key + key.destroy + else + not_found!('Deploy Key') + end end end end -- GitLab From d7755ede246988e3186a46b2c9fbd1b70660b529 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 4 Jan 2017 19:13:29 +0000 Subject: [PATCH 35/40] Merge branch 'fix/rename-group-export-vuln' into 'security' Fix export files not removed when a user takes over a namespace See merge request !2051 --- app/models/namespace.rb | 20 ++++++ .../namespace_export_file_spec.rb | 62 +++++++++++++++++++ spec/models/namespace_spec.rb | 11 +++- 3 files changed, 91 insertions(+), 2 deletions(-) create mode 100644 spec/features/projects/import_export/namespace_export_file_spec.rb diff --git a/app/models/namespace.rb b/app/models/namespace.rb index d41833de66f2cd..dd33975731f16a 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -130,6 +130,8 @@ def move_dir Gitlab::UploadsTransfer.new.rename_namespace(path_was, path) + remove_exports! + # If repositories moved successfully we need to # send update instructions to users. # However we cannot allow rollback since we moved namespace dir @@ -214,6 +216,8 @@ def rm_dir GitlabShellWorker.perform_in(5.minutes, :rm_namespace, repository_storage_path, new_path) end end + + remove_exports! end def refresh_access_of_projects_invited_groups @@ -226,4 +230,20 @@ def refresh_access_of_projects_invited_groups def full_path_changed? path_changed? || parent_id_changed? end + + def remove_exports! + Gitlab::Popen.popen(%W(find #{export_path} -not -path #{export_path} -delete)) + end + + def export_path + File.join(Gitlab::ImportExport.storage_path, full_path_was) + end + + def full_path_was + if parent + parent.full_path + '/' + path_was + else + path_was + end + end end diff --git a/spec/features/projects/import_export/namespace_export_file_spec.rb b/spec/features/projects/import_export/namespace_export_file_spec.rb new file mode 100644 index 00000000000000..d0bafc6168cf7a --- /dev/null +++ b/spec/features/projects/import_export/namespace_export_file_spec.rb @@ -0,0 +1,62 @@ +require 'spec_helper' + +feature 'Import/Export - Namespace export file cleanup', feature: true, js: true do + let(:export_path) { "#{Dir::tmpdir}/import_file_spec" } + let(:config_hash) { YAML.load_file(Gitlab::ImportExport.config_file).deep_stringify_keys } + + let(:project) { create(:empty_project) } + + background do + allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path) + end + + after do + FileUtils.rm_rf(export_path, secure: true) + end + + context 'admin user' do + before do + login_as(:admin) + end + + context 'moving the namespace' do + scenario 'removes the export file' do + setup_export_project + + old_export_path = project.export_path.dup + + expect(File).to exist(old_export_path) + + project.namespace.update(path: 'new_path') + + expect(File).not_to exist(old_export_path) + end + end + + context 'deleting the namespace' do + scenario 'removes the export file' do + setup_export_project + + old_export_path = project.export_path.dup + + expect(File).to exist(old_export_path) + + project.namespace.destroy + + expect(File).not_to exist(old_export_path) + end + end + + def setup_export_project + visit edit_namespace_project_path(project.namespace, project) + + expect(page).to have_content('Export project') + + click_link 'Export project' + + visit edit_namespace_project_path(project.namespace, project) + + expect(page).to have_content('Download export') + end + end +end diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 600538ff5f435f..f8e03fa114a8ca 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -117,6 +117,7 @@ new_path = @namespace.path + "_new" allow(@namespace).to receive(:path_was).and_return(@namespace.path) allow(@namespace).to receive(:path).and_return(new_path) + expect(@namespace).to receive(:remove_exports!) expect(@namespace.move_dir).to be_truthy end @@ -139,11 +140,17 @@ let!(:project) { create(:project, namespace: namespace) } let!(:path) { File.join(Gitlab.config.repositories.storages.default, namespace.path) } - before { namespace.destroy } - it "removes its dirs when deleted" do + namespace.destroy + expect(File.exist?(path)).to be(false) end + + it 'removes the exports folder' do + expect(namespace).to receive(:remove_exports!) + + namespace.destroy + end end describe '.find_by_path_or_name' do -- GitLab From 3a5df1d8fc518900d8e33a6be8a2243e399c754a Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 3 Jan 2017 18:03:13 +0000 Subject: [PATCH 36/40] Merge branch 'fix-api-mr-permissions' into 'security' Ensure that only privileged users can access merge requests in the API See merge request !2053 --- .../unreleased/fix-api-mr-permissions.yml | 4 +++ lib/api/helpers.rb | 6 ++++ lib/api/merge_request_diffs.rb | 8 ++--- lib/api/merge_requests.rb | 25 +++++++--------- lib/api/subscriptions.rb | 4 +-- lib/api/todos.rb | 2 +- spec/requests/api/merge_requests_spec.rb | 29 +++++++++++++++++++ spec/requests/api/todos_spec.rb | 15 +++++++++- 8 files changed, 68 insertions(+), 25 deletions(-) create mode 100644 changelogs/unreleased/fix-api-mr-permissions.yml diff --git a/changelogs/unreleased/fix-api-mr-permissions.yml b/changelogs/unreleased/fix-api-mr-permissions.yml new file mode 100644 index 00000000000000..33b677b1f2919f --- /dev/null +++ b/changelogs/unreleased/fix-api-mr-permissions.yml @@ -0,0 +1,4 @@ +--- +title: Don't allow project guests to subscribe to merge requests through the API +merge_request: +author: Robert Schilling diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 49c5f0652ab58b..a1d7b323f4f14e 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -90,6 +90,12 @@ def find_project_merge_request(id) MergeRequestsFinder.new(current_user, project_id: user_project.id).find(id) end + def find_merge_request_with_access(id, access_level = :read_merge_request) + merge_request = user_project.merge_requests.find(id) + authorize! access_level, merge_request + merge_request + end + def authenticate! unauthorized! unless current_user end diff --git a/lib/api/merge_request_diffs.rb b/lib/api/merge_request_diffs.rb index 07435d78468ea3..bc3d69f6904b6d 100644 --- a/lib/api/merge_request_diffs.rb +++ b/lib/api/merge_request_diffs.rb @@ -15,10 +15,8 @@ class MergeRequestDiffs < Grape::API end get ":id/merge_requests/:merge_request_id/versions" do - merge_request = user_project.merge_requests. - find(params[:merge_request_id]) + merge_request = find_merge_request_with_access(params[:merge_request_id]) - authorize! :read_merge_request, merge_request present merge_request.merge_request_diffs, with: Entities::MergeRequestDiff end @@ -34,10 +32,8 @@ class MergeRequestDiffs < Grape::API end get ":id/merge_requests/:merge_request_id/versions/:version_id" do - merge_request = user_project.merge_requests. - find(params[:merge_request_id]) + merge_request = find_merge_request_with_access(params[:merge_request_id]) - authorize! :read_merge_request, merge_request present merge_request.merge_request_diffs.find(params[:version_id]), with: Entities::MergeRequestDiffFull end end diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index e77af4b7a0d377..7ffb38e62daa3f 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -118,8 +118,8 @@ def handle_merge_request_errors!(errors) success Entities::MergeRequest end get path do - merge_request = find_project_merge_request(params[:merge_request_id]) - authorize! :read_merge_request, merge_request + merge_request = find_merge_request_with_access(params[:merge_request_id]) + present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project end @@ -127,8 +127,8 @@ def handle_merge_request_errors!(errors) success Entities::RepoCommit end get "#{path}/commits" do - merge_request = find_project_merge_request(params[:merge_request_id]) - authorize! :read_merge_request, merge_request + merge_request = find_merge_request_with_access(params[:merge_request_id]) + present merge_request.commits, with: Entities::RepoCommit end @@ -136,8 +136,8 @@ def handle_merge_request_errors!(errors) success Entities::MergeRequestChanges end get "#{path}/changes" do - merge_request = find_project_merge_request(params[:merge_request_id]) - authorize! :read_merge_request, merge_request + merge_request = find_merge_request_with_access(params[:merge_request_id]) + present merge_request, with: Entities::MergeRequestChanges, current_user: current_user end @@ -155,8 +155,7 @@ def handle_merge_request_errors!(errors) :remove_source_branch end put path do - merge_request = find_project_merge_request(params.delete(:merge_request_id)) - authorize! :update_merge_request, merge_request + merge_request = find_merge_request_with_access(params.delete(:merge_request_id), :update_merge_request) mr_params = declared_params(include_missing: false) mr_params[:force_remove_source_branch] = mr_params.delete(:remove_source_branch) if mr_params[:remove_source_branch].present? @@ -235,10 +234,7 @@ def handle_merge_request_errors!(errors) use :pagination end get "#{path}/comments" do - merge_request = find_project_merge_request(params[:merge_request_id]) - - authorize! :read_merge_request, merge_request - + merge_request = find_merge_request_with_access(params[:merge_request_id]) present paginate(merge_request.notes.fresh), with: Entities::MRNote end @@ -250,8 +246,7 @@ def handle_merge_request_errors!(errors) requires :note, type: String, desc: 'The text of the comment' end post "#{path}/comments" do - merge_request = find_project_merge_request(params[:merge_request_id]) - authorize! :create_note, merge_request + merge_request = find_merge_request_with_access(params[:merge_request_id], :create_note) opts = { note: params[:note], @@ -275,7 +270,7 @@ def handle_merge_request_errors!(errors) use :pagination end get "#{path}/closes_issues" do - merge_request = find_project_merge_request(params[:merge_request_id]) + merge_request = find_merge_request_with_access(params[:merge_request_id]) issues = ::Kaminari.paginate_array(merge_request.closes_issues(current_user)) present paginate(issues), with: issue_entity(user_project), current_user: current_user end diff --git a/lib/api/subscriptions.rb b/lib/api/subscriptions.rb index 10749b34004fe8..e11d7537cc9c9f 100644 --- a/lib/api/subscriptions.rb +++ b/lib/api/subscriptions.rb @@ -3,8 +3,8 @@ class Subscriptions < Grape::API before { authenticate! } subscribable_types = { - 'merge_request' => proc { |id| user_project.merge_requests.find(id) }, - 'merge_requests' => proc { |id| user_project.merge_requests.find(id) }, + 'merge_request' => proc { |id| find_merge_request_with_access(id, :update_merge_request) }, + 'merge_requests' => proc { |id| find_merge_request_with_access(id, :update_merge_request) }, 'issues' => proc { |id| find_project_issue(id) }, 'labels' => proc { |id| find_project_label(id) }, } diff --git a/lib/api/todos.rb b/lib/api/todos.rb index ed8f48aa1e3b3a..9bd077263a7e01 100644 --- a/lib/api/todos.rb +++ b/lib/api/todos.rb @@ -5,7 +5,7 @@ class Todos < Grape::API before { authenticate! } ISSUABLE_TYPES = { - 'merge_requests' => ->(id) { user_project.merge_requests.find(id) }, + 'merge_requests' => ->(id) { find_merge_request_with_access(id) }, 'issues' => ->(id) { find_project_issue(id) } } diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 6f20ac4926949c..71a7994e544c32 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -627,6 +627,17 @@ expect(json_response.first['title']).to eq(issue.title) expect(json_response.first['id']).to eq(issue.id) end + + it 'returns 403 if the user has no access to the merge request' do + project = create(:empty_project, :private) + merge_request = create(:merge_request, :simple, source_project: project) + guest = create(:user) + project.team << [guest, :guest] + + get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/closes_issues", guest) + + expect(response).to have_http_status(403) + end end describe 'POST :id/merge_requests/:merge_request_id/subscription' do @@ -648,6 +659,15 @@ expect(response).to have_http_status(404) end + + it 'returns 403 if user has no access to read code' do + guest = create(:user) + project.team << [guest, :guest] + + post api("/projects/#{project.id}/merge_requests/#{merge_request.id}/subscription", guest) + + expect(response).to have_http_status(403) + end end describe 'DELETE :id/merge_requests/:merge_request_id/subscription' do @@ -669,6 +689,15 @@ expect(response).to have_http_status(404) end + + it 'returns 403 if user has no access to read code' do + guest = create(:user) + project.team << [guest, :guest] + + delete api("/projects/#{project.id}/merge_requests/#{merge_request.id}/subscription", guest) + + expect(response).to have_http_status(403) + end end describe 'Time tracking' do diff --git a/spec/requests/api/todos_spec.rb b/spec/requests/api/todos_spec.rb index 6fe695626caa76..56dc017ce54ba8 100644 --- a/spec/requests/api/todos_spec.rb +++ b/spec/requests/api/todos_spec.rb @@ -183,12 +183,25 @@ expect(response.status).to eq(404) end + + it 'returns an error if the issuable is not accessible' do + guest = create(:user) + project_1.team << [guest, :guest] + + post api("/projects/#{project_1.id}/#{issuable_type}/#{issuable.id}/todo", guest) + + if issuable_type == 'merge_requests' + expect(response).to have_http_status(403) + else + expect(response).to have_http_status(404) + end + end end describe 'POST :id/issuable_type/:issueable_id/todo' do context 'for an issue' do it_behaves_like 'an issuable', 'issues' do - let(:issuable) { create(:issue, author: author_1, project: project_1) } + let(:issuable) { create(:issue, :confidential, author: author_1, project: project_1) } end end -- GitLab From a1f959430b752aca21f798d37d338e11afaa6110 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 19 Jan 2017 21:00:47 +0000 Subject: [PATCH 37/40] Merge branch 'fix-guest-access-posting-to-notes' into 'security' Prevent users from creating notes on resources they can't access See https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2054 --- .../fix-guest-access-posting-to-notes.yml | 4 +++ lib/api/notes.rb | 26 ++++++++++++------- spec/requests/api/notes_spec.rb | 12 +++++++++ 3 files changed, 32 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/fix-guest-access-posting-to-notes.yml diff --git a/changelogs/unreleased/fix-guest-access-posting-to-notes.yml b/changelogs/unreleased/fix-guest-access-posting-to-notes.yml new file mode 100644 index 00000000000000..81377c0c6f0814 --- /dev/null +++ b/changelogs/unreleased/fix-guest-access-posting-to-notes.yml @@ -0,0 +1,4 @@ +--- +title: Prevent users from creating notes on resources they can't access +merge_request: +author: diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 284e4cf549ad9f..4d2a8f482670c3 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -70,21 +70,27 @@ class Notes < Grape::API end post ":id/#{noteables_str}/:noteable_id/notes" do opts = { - note: params[:body], - noteable_type: noteables_str.classify, - noteable_id: params[:noteable_id] + note: params[:body], + noteable_type: noteables_str.classify, + noteable_id: params[:noteable_id] } - if params[:created_at] && (current_user.is_admin? || user_project.owner == current_user) - opts[:created_at] = params[:created_at] - end + noteable = user_project.send(noteables_str.to_sym).find(params[:noteable_id]) + + if can?(current_user, noteable_read_ability_name(noteable), noteable) + if params[:created_at] && (current_user.is_admin? || user_project.owner == current_user) + opts[:created_at] = params[:created_at] + end - note = ::Notes::CreateService.new(user_project, current_user, opts).execute + note = ::Notes::CreateService.new(user_project, current_user, opts).execute - if note.valid? - present note, with: Entities::const_get(note.class.name) + if note.valid? + present note, with: Entities::const_get(note.class.name) + else + not_found!("Note #{note.errors.messages}") + end else - not_found!("Note #{note.errors.messages}") + not_found!("Note") end end diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index 0f8d054b31e977..0353ebea9e59b3 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -264,6 +264,18 @@ end end + context 'when user does not have access to read the noteable' do + it 'responds with 404' do + project = create(:empty_project, :private) { |p| p.add_guest(user) } + issue = create(:issue, :confidential, project: project) + + post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), + body: 'Foo' + + expect(response).to have_http_status(404) + end + end + context 'when user does not have access to create noteable' do let(:private_issue) { create(:issue, project: create(:empty_project, :private)) } -- GitLab From ec3226bd60f545c8ffb231011c45afa0bfc66e86 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 19 Jan 2017 17:49:16 +0000 Subject: [PATCH 38/40] Merge branch 'upgrade-omniauth' into 'security' Upgrade OmniAuth Ruby gem to 1.3.2 Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/26813 See merge request !2056 --- Gemfile | 2 +- Gemfile.lock | 4 ++-- changelogs/unreleased/upgrade-omniauth.yml | 4 ++++ 3 files changed, 7 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/upgrade-omniauth.yml diff --git a/Gemfile b/Gemfile index 83ba5d31b92fc2..4e9cf91c42952d 100644 --- a/Gemfile +++ b/Gemfile @@ -21,7 +21,7 @@ gem 'rugged', '~> 0.24.0' # Authentication libraries gem 'devise', '~> 4.2' gem 'doorkeeper', '~> 4.2.0' -gem 'omniauth', '~> 1.3.1' +gem 'omniauth', '~> 1.3.2' gem 'omniauth-auth0', '~> 1.4.1' gem 'omniauth-azure-oauth2', '~> 0.0.6' gem 'omniauth-cas3', '~> 1.1.2' diff --git a/Gemfile.lock b/Gemfile.lock index 104e6444803e13..c9115982838657 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -449,7 +449,7 @@ GEM octokit (4.6.2) sawyer (~> 0.8.0, >= 0.5.3) oj (2.17.4) - omniauth (1.3.1) + omniauth (1.3.2) hashie (>= 1.2, < 4) rack (>= 1.0, < 3) omniauth-auth0 (1.4.1) @@ -925,7 +925,7 @@ DEPENDENCIES oauth2 (~> 1.2.0) octokit (~> 4.6.2) oj (~> 2.17.4) - omniauth (~> 1.3.1) + omniauth (~> 1.3.2) omniauth-auth0 (~> 1.4.1) omniauth-authentiq (~> 0.2.0) omniauth-azure-oauth2 (~> 0.0.6) diff --git a/changelogs/unreleased/upgrade-omniauth.yml b/changelogs/unreleased/upgrade-omniauth.yml new file mode 100644 index 00000000000000..7e0334566dcaa9 --- /dev/null +++ b/changelogs/unreleased/upgrade-omniauth.yml @@ -0,0 +1,4 @@ +--- +title: Upgrade omniauth gem to 1.3.2 +merge_request: +author: -- GitLab From 76deb55f5602ffb137d6e1daf17212e36efb52bf Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 20 Jan 2017 13:28:27 -0500 Subject: [PATCH 39/40] Add a changelog entry for #26242 --- changelogs/unreleased/8-15-stable.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/8-15-stable.yml diff --git a/changelogs/unreleased/8-15-stable.yml b/changelogs/unreleased/8-15-stable.yml new file mode 100644 index 00000000000000..75502e139e7b3c --- /dev/null +++ b/changelogs/unreleased/8-15-stable.yml @@ -0,0 +1,4 @@ +--- +title: Ensure export files are removed after a namespace is deleted +merge_request: +author: -- GitLab From ffa56e537f15989711f4240a2f634f50701b8dcc Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Tue, 24 Jan 2017 14:30:41 +0000 Subject: [PATCH 40/40] fixes test suite related to the service api --- app/models/project_services/jenkins_deprecated_service.rb | 2 +- app/models/project_services/jenkins_service.rb | 4 ++-- lib/api/services.rb | 2 ++ 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/models/project_services/jenkins_deprecated_service.rb b/app/models/project_services/jenkins_deprecated_service.rb index 02781f04d6bd3b..46873339e15cc6 100644 --- a/app/models/project_services/jenkins_deprecated_service.rb +++ b/app/models/project_services/jenkins_deprecated_service.rb @@ -33,7 +33,7 @@ def help 'is deprecated. Use "Jenkins CI" service instead.' end - def to_param + def self.to_param 'jenkins_deprecated' end diff --git a/app/models/project_services/jenkins_service.rb b/app/models/project_services/jenkins_service.rb index 9c4e229ad27c71..992e75b7717046 100644 --- a/app/models/project_services/jenkins_service.rb +++ b/app/models/project_services/jenkins_service.rb @@ -52,7 +52,7 @@ def hook_url File.join(jenkins_url, "project/#{project_name}").to_s end - def supported_events + def self.supported_events %w(push merge_request tag_push) end @@ -68,7 +68,7 @@ def help 'You must have installed the Git Plugin and GitLab Plugin in Jenkins' end - def to_param + def self.to_param 'jenkins' end diff --git a/lib/api/services.rb b/lib/api/services.rb index eee31ed9044a26..4781acc3b27778 100644 --- a/lib/api/services.rb +++ b/lib/api/services.rb @@ -609,6 +609,8 @@ class Services < Grape::API SlackService, MattermostService, TeamcityService, + JenkinsService, + JenkinsDeprecatedService ].freeze trigger_services = { -- GitLab