From 52867e15acacf842e26816c9143c59fc9086c6fb Mon Sep 17 00:00:00 2001 From: Brian Neel Date: Mon, 14 Nov 2016 20:30:12 -0500 Subject: [PATCH 001/130] Disable automatic login feature when clicking on email confirmation links --- app/controllers/confirmations_controller.rb | 8 ++------ .../disable-autologin-on-email-confirmation-links.yml | 4 ++++ 2 files changed, 6 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/disable-autologin-on-email-confirmation-links.yml diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb index 3da44b9b888..306afb65f10 100644 --- a/app/controllers/confirmations_controller.rb +++ b/app/controllers/confirmations_controller.rb @@ -14,12 +14,8 @@ class ConfirmationsController < Devise::ConfirmationsController if signed_in?(resource_name) after_sign_in_path_for(resource) else - sign_in(resource) - if signed_in?(resource_name) - after_sign_in_path_for(resource) - else - new_session_path(resource_name) - end + flash[:notice] += " Please sign in." + new_session_path(resource_name) end end end diff --git a/changelogs/unreleased/disable-autologin-on-email-confirmation-links.yml b/changelogs/unreleased/disable-autologin-on-email-confirmation-links.yml new file mode 100644 index 00000000000..6dd0d748001 --- /dev/null +++ b/changelogs/unreleased/disable-autologin-on-email-confirmation-links.yml @@ -0,0 +1,4 @@ +--- +title: Disable automatic login after clicking email confirmation links +merge_request: 7472 +author: From 8b30dd9834fd4026b846b016868701d8e95ec048 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 9 Jan 2017 11:46:15 +0100 Subject: [PATCH 002/130] 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 16dcb326be9..31e56a485b7 100644 --- a/lib/gitlab/ci/status/pipeline/factory.rb +++ b/lib/gitlab/ci/status/pipeline/factory.rb @@ -4,7 +4,7 @@ module Gitlab 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 00000000000..8387682e744 --- /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 24bf8b869e0..00000000000 --- 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 00000000000..2affcc08f50 --- /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 d4a2dc7fcc1..672f5733e98 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 @@ describe Gitlab::Ci::Status::Pipeline::Factory do 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 979160eb9c4..ee3f6174b73 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 From 8dbd1e7d0000bb08b6ac6867530bb501eadc85a4 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 9 Jan 2017 12:22:19 +0100 Subject: [PATCH 003/130] 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 d035eda6df5..d4b6ff910aa 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -39,5 +39,13 @@ module Ci 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 31e56a485b7..13c8343b12a 100644 --- a/lib/gitlab/ci/status/pipeline/factory.rb +++ b/lib/gitlab/ci/status/pipeline/factory.rb @@ -4,7 +4,7 @@ module Gitlab 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 8387682e744..00000000000 --- 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 689a5dd45bc..4c37f084d07 100644 --- a/lib/gitlab/ci/status/stage/factory.rb +++ b/lib/gitlab/ci/status/stage/factory.rb @@ -3,6 +3,10 @@ module Gitlab 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 2affcc08f50..d4cdab6957a 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 @@ module Gitlab 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 672f5733e98..0df6e881877 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 @@ describe Gitlab::Ci::Status::Pipeline::Factory do 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 ee3f6174b73..00000000000 --- 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 6f8721d30c2..a60f84be9e9 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 @@ describe Gitlab::Ci::Status::Stage::Factory do 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 00000000000..7e2269397c6 --- /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 From fd115bc130a8bf77814262b67eb0f20f1f19cb85 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 9 Jan 2017 13:07:52 +0100 Subject: [PATCH 004/130] 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 a60f84be9e9..bbb40e2c1ab 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 @@ describe Gitlab::Ci::Status::Stage::Factory do 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 742bedb37e4..3d387d52c8e 100644 --- a/spec/models/ci/stage_spec.rb +++ b/spec/models/ci/stage_spec.rb @@ -142,6 +142,54 @@ describe Ci::Stage, models: true do 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 From e5d53df70e6bebf267e5709790842cb674ee96b0 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 9 Jan 2017 14:13:16 +0100 Subject: [PATCH 005/130] 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 00000000000..5fba0332881 --- /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: From 9f1279184b85927a12b93df00896c57b12373a9a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 11 Jan 2017 13:26:56 +0100 Subject: [PATCH 006/130] 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 00000000000..807afe24bd5 --- /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 00000000000..20f71459738 --- /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 From 1d01ffb782a1bf44d5826666590408b24fba2332 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 12 Jan 2017 12:45:00 +0100 Subject: [PATCH 007/130] 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 ae9ef895df4..71c54aebcc3 100644 --- a/lib/gitlab/ci/status/factory.rb +++ b/lib/gitlab/ci/status/factory.rb @@ -8,10 +8,12 @@ module Gitlab 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 @@ module Gitlab .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 f92a1c149bf..d78d563a9b9 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 @@ describe Gitlab::Ci::Status::Factory do 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 From 48c19e7395e501c8e9eaa3975b2510b37f56d46c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 12 Jan 2017 13:04:51 +0100 Subject: [PATCH 008/130] 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 71c54aebcc3..3da5ae4fc01 100644 --- a/lib/gitlab/ci/status/factory.rb +++ b/lib/gitlab/ci/status/factory.rb @@ -5,6 +5,7 @@ module Gitlab def initialize(subject, user) @subject = subject @user = user + @status = subject.status || :created end def fabricate! @@ -17,23 +18,9 @@ module Gitlab 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 @@ module Gitlab @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 d78d563a9b9..f1b758640a7 100644 --- a/spec/lib/gitlab/ci/status/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/factory_spec.rb @@ -2,17 +2,28 @@ require 'spec_helper' 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 @@ describe Gitlab::Ci::Status::Factory do 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 @@ describe Gitlab::Ci::Status::Factory do .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 From 6053049f4a8822bf39bbe8e1fa34c5e3522b63e9 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 12 Jan 2017 13:22:04 +0100 Subject: [PATCH 009/130] 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 dccf5177e35..868f28cd356 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 f19275770be..6f31d4ed789 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; From 227cbdd8ba969aecdbee68f1c892c85ed196d64e Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 12 Jan 2017 13:22:46 +0100 Subject: [PATCH 010/130] 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 eee9a64120b..38ac6edc9f1 100644 --- a/lib/gitlab/ci/status/build/factory.rb +++ b/lib/gitlab/ci/status/build/factory.rb @@ -4,8 +4,11 @@ module Gitlab 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 dccb29b5ef6..0c40fca0c1a 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 @@ require 'spec_helper' 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 @@ describe Gitlab::Ci::Status::Build::Factory do 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 @@ describe Gitlab::Ci::Status::Build::Factory do 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 @@ describe Gitlab::Ci::Status::Build::Factory do 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 @@ describe Gitlab::Ci::Status::Build::Factory do 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 @@ describe Gitlab::Ci::Status::Build::Factory do 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 @@ describe Gitlab::Ci::Status::Build::Factory do 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 From 528c06882560eb14d14babf5cf5bb73d2276cb0c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 12 Jan 2017 13:26:21 +0100 Subject: [PATCH 011/130] 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 f1b758640a7..bbf9c7c83a3 100644 --- a/spec/lib/gitlab/ci/status/factory_spec.rb +++ b/spec/lib/gitlab/ci/status/factory_spec.rb @@ -11,7 +11,7 @@ describe Gitlab::Ci::Status::Factory do 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 From ee18d89f3dc2b00f7e9514e21c12139ecc8f9224 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 12 Jan 2017 13:46:53 +0100 Subject: [PATCH 012/130] 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 0df6e881877..b10a447c27a 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 @@ require 'spec_helper' 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 "fabricates a core status #{core_status}" do - expect(status).to be_a( - Gitlab::Ci::Status.const_get(core_status.capitalize)) + it "matches correct core status for #{simple_status}" do + expect(factory.core_status).to be_a expected_status + end + + 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 @@ describe Gitlab::Ci::Status::Pipeline::Factory do 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 From c3a940000ea20d6682313640e1a0fda9ff68dbdf Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Wed, 12 Oct 2016 19:07:36 +0200 Subject: [PATCH 013/130] 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 0bc1c19e9cd..0cd3456b4de 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -107,15 +107,11 @@ class Notify < BaseMailer 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 @@ class Notify < BaseMailer 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 00000000000..f4011b756a5 --- /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 bd3267e2a80..bd2f5d3615e 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 7cccf465334..3f6ace0311a 100644 --- a/lib/gitlab/email/handler/base_handler.rb +++ b/lib/gitlab/email/handler/base_handler.rb @@ -9,52 +9,13 @@ module Gitlab @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 9f90a3ec2b2..127fae159d5 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 447c7a6a6b9..d87ba427f4b 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 @@ module Gitlab record_name: 'comment') end + private + def author sent_notification.recipient end @@ -36,8 +41,6 @@ module Gitlab @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 00000000000..32c5caf93e8 --- /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 00000000000..97d7a8d65ff --- /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 801dfde9a36..b91012d6405 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 @@ module Gitlab 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 @@ module Gitlab 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 19298e261e3..9d806fc524d 100644 --- a/spec/lib/gitlab/email/email_shared_blocks.rb +++ b/spec/lib/gitlab/email/email_shared_blocks.rb @@ -18,7 +18,7 @@ shared_context :email_shared_context do 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 cb3651e3845..08897a4c310 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 @@ require_relative '../email_shared_blocks' 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 48660d1dd1b..cebbeff50cf 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 @@ require_relative '../email_shared_blocks' 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 00000000000..a444257754b --- /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 1dcf2c0668b..7e951e3fcdd 100644 --- a/spec/lib/gitlab/incoming_email_spec.rb +++ b/spec/lib/gitlab/incoming_email_spec.rb @@ -23,6 +23,48 @@ describe Gitlab::IncomingEmail, lib: true do 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 49867aa5cc4..a3724b801b3 100644 --- a/spec/support/notify_shared_examples.rb +++ b/spec/support/notify_shared_examples.rb @@ -179,9 +179,24 @@ shared_examples 'it should show Gmail Actions View Commit link' do 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', /^<.+>$/ + 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.not_to have_header 'List-Unsubscribe', /mailto/ + is_expected.to have_header 'List-Unsubscribe', /^<[^,]+>$/ end it { is_expected.to have_body_text /unsubscribe/ } From eb9b96405498e37b25aa32876b0e101d1880f4e9 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Thu, 5 Jan 2017 12:40:54 +0100 Subject: [PATCH 014/130] 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 00000000000..2c6883bcf7b --- /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 6c7e673fb9f..6ce9b229294 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -35,7 +35,9 @@ module Gitlab 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 d3c3b800b94..369e55f61f1 100644 --- a/spec/lib/gitlab/user_access_spec.rb +++ b/spec/lib/gitlab/user_access_spec.rb @@ -66,7 +66,8 @@ describe Gitlab::UserAccess, lib: true do 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 @@ describe Gitlab::UserAccess, lib: true do 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 From e2585e642ac53802c6c5b4c2ee2ec3e6be584981 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 13 Jan 2017 16:12:02 +0000 Subject: [PATCH 015/130] 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 1ef34c79971..e4a0e0b92bc 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 0363bf8c1d5..8232a0a113c 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 From b038c53073b191df2044f96d4ff5d01a35b22d83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Sat, 14 Jan 2017 00:18:21 -0500 Subject: [PATCH 016/130] Move default values to ApplicationSetting::DEFAULTS and use it in CurrentSettings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/models/application_setting.rb | 84 ++++++++++++++++--------------- lib/gitlab/current_settings.rb | 30 +---------- 2 files changed, 45 insertions(+), 69 deletions(-) diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 8fab77cda0a..e33a58d3771 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -13,6 +13,49 @@ class ApplicationSetting < ActiveRecord::Base [\r\n] # any number of newline characters }x + DEFAULTS_CE = { + after_sign_up_text: nil, + akismet_enabled: false, + container_registry_token_expire_delay: 5, + default_branch_protection: Settings.gitlab['default_branch_protection'], + default_project_visibility: Settings.gitlab.default_projects_features['visibility_level'], + default_projects_limit: Settings.gitlab['default_projects_limit'], + default_snippet_visibility: Settings.gitlab.default_projects_features['visibility_level'], + disabled_oauth_sign_in_sources: [], + domain_whitelist: Settings.gitlab['domain_whitelist'], + gravatar_enabled: Settings.gravatar['enabled'], + help_page_text: nil, + housekeeping_bitmaps_enabled: true, + housekeeping_enabled: true, + housekeeping_full_repack_period: 50, + housekeeping_gc_period: 200, + housekeeping_incremental_repack_period: 10, + import_sources: Gitlab::ImportSources.values, + koding_enabled: false, + koding_url: nil, + max_artifacts_size: Settings.artifacts['max_size'], + max_attachment_size: Settings.gitlab['max_attachment_size'], + plantuml_enabled: false, + plantuml_url: nil, + recaptcha_enabled: false, + repository_checks_enabled: true, + repository_storages: ['default'], + require_two_factor_authentication: false, + restricted_visibility_levels: Settings.gitlab['restricted_visibility_levels'], + session_expire_delay: Settings.gitlab['session_expire_delay'], + send_user_confirmation_email: false, + shared_runners_enabled: Settings.gitlab_ci['shared_runners_enabled'], + shared_runners_text: nil, + sidekiq_throttling_enabled: false, + sign_in_text: nil, + signin_enabled: Settings.gitlab['signin_enabled'], + signup_enabled: Settings.gitlab['signup_enabled'], + two_factor_grace_period: 48, + user_default_external: false + } + + DEFAULTS = DEFAULTS_CE + serialize :restricted_visibility_levels serialize :import_sources serialize :disabled_oauth_sign_in_sources, Array @@ -163,46 +206,7 @@ class ApplicationSetting < ActiveRecord::Base end def self.create_from_defaults - create( - default_projects_limit: Settings.gitlab['default_projects_limit'], - default_branch_protection: Settings.gitlab['default_branch_protection'], - signup_enabled: Settings.gitlab['signup_enabled'], - signin_enabled: Settings.gitlab['signin_enabled'], - gravatar_enabled: Settings.gravatar['enabled'], - sign_in_text: nil, - after_sign_up_text: nil, - help_page_text: nil, - shared_runners_text: nil, - restricted_visibility_levels: Settings.gitlab['restricted_visibility_levels'], - max_attachment_size: Settings.gitlab['max_attachment_size'], - session_expire_delay: Settings.gitlab['session_expire_delay'], - default_project_visibility: Settings.gitlab.default_projects_features['visibility_level'], - default_snippet_visibility: Settings.gitlab.default_projects_features['visibility_level'], - domain_whitelist: Settings.gitlab['domain_whitelist'], - import_sources: Gitlab::ImportSources.values, - shared_runners_enabled: Settings.gitlab_ci['shared_runners_enabled'], - max_artifacts_size: Settings.artifacts['max_size'], - require_two_factor_authentication: false, - two_factor_grace_period: 48, - recaptcha_enabled: false, - akismet_enabled: false, - koding_enabled: false, - koding_url: nil, - plantuml_enabled: false, - plantuml_url: nil, - repository_checks_enabled: true, - disabled_oauth_sign_in_sources: [], - send_user_confirmation_email: false, - container_registry_token_expire_delay: 5, - repository_storages: ['default'], - user_default_external: false, - sidekiq_throttling_enabled: false, - housekeeping_enabled: true, - housekeeping_bitmaps_enabled: true, - housekeeping_incremental_repack_period: 10, - housekeeping_full_repack_period: 50, - housekeeping_gc_period: 200, - ) + create(DEFAULTS) end def home_page_url_column_exist diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index 2ff27e46d64..c4fc3709c93 100644 --- a/lib/gitlab/current_settings.rb +++ b/lib/gitlab/current_settings.rb @@ -28,35 +28,7 @@ module Gitlab end def fake_application_settings - OpenStruct.new( - default_projects_limit: Settings.gitlab['default_projects_limit'], - default_branch_protection: Settings.gitlab['default_branch_protection'], - signup_enabled: Settings.gitlab['signup_enabled'], - signin_enabled: Settings.gitlab['signin_enabled'], - gravatar_enabled: Settings.gravatar['enabled'], - koding_enabled: false, - plantuml_enabled: false, - sign_in_text: nil, - after_sign_up_text: nil, - help_page_text: nil, - shared_runners_text: nil, - restricted_visibility_levels: Settings.gitlab['restricted_visibility_levels'], - max_attachment_size: Settings.gitlab['max_attachment_size'], - session_expire_delay: Settings.gitlab['session_expire_delay'], - default_project_visibility: Settings.gitlab.default_projects_features['visibility_level'], - default_snippet_visibility: Settings.gitlab.default_projects_features['visibility_level'], - domain_whitelist: Settings.gitlab['domain_whitelist'], - import_sources: %w[gitea github bitbucket gitlab google_code fogbugz git gitlab_project], - shared_runners_enabled: Settings.gitlab_ci['shared_runners_enabled'], - max_artifacts_size: Settings.artifacts['max_size'], - require_two_factor_authentication: false, - two_factor_grace_period: 48, - akismet_enabled: false, - repository_checks_enabled: true, - container_registry_token_expire_delay: 5, - user_default_external: false, - sidekiq_throttling_enabled: false, - ) + ApplicationSetting.new(ApplicationSetting::DEFAULTS) end private From f6cc29ed83921c3dce98d8c519c4826e7cc8221a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Sat, 14 Jan 2017 00:18:40 -0500 Subject: [PATCH 017/130] Don't persist ApplicationSetting in test env MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- lib/gitlab/current_settings.rb | 16 ++++- lib/gitlab/github_import/project_creator.rb | 4 +- .../health_check_controller_spec.rb | 6 ++ ...admin_disables_git_access_protocol_spec.rb | 3 + .../features/admin/admin_health_check_spec.rb | 9 ++- spec/features/admin/admin_runners_spec.rb | 3 + spec/features/admin/admin_settings_spec.rb | 5 +- .../admin_uses_repository_checks_spec.rb | 9 ++- spec/lib/gitlab/current_settings_spec.rb | 70 +++++++++++++------ spec/requests/api/internal_spec.rb | 9 +-- spec/spec_helper.rb | 1 + 11 files changed, 98 insertions(+), 37 deletions(-) diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index c4fc3709c93..c79e17b57ee 100644 --- a/lib/gitlab/current_settings.rb +++ b/lib/gitlab/current_settings.rb @@ -9,7 +9,9 @@ module Gitlab end def ensure_application_settings! - if connect_to_db? + return fake_application_settings unless connect_to_db? + + unless ENV['IN_MEMORY_APPLICATION_SETTINGS'] == 'true' begin settings = ::ApplicationSetting.current # In case Redis isn't running or the Redis UNIX socket file is not available @@ -20,15 +22,23 @@ module Gitlab settings ||= ::ApplicationSetting.create_from_defaults unless ActiveRecord::Migrator.needs_migration? end - settings || fake_application_settings + settings || in_memory_application_settings end def sidekiq_throttling_enabled? current_application_settings.sidekiq_throttling_enabled? end + def in_memory_application_settings + @in_memory_application_settings ||= ApplicationSetting.new(ApplicationSetting::DEFAULTS) + # In case migrations the application_settings table is not created yet, + # we fallback to a simple OpenStruct + rescue ActiveRecord::StatementInvalid + fake_application_settings + end + def fake_application_settings - ApplicationSetting.new(ApplicationSetting::DEFAULTS) + OpenStruct.new(ApplicationSetting::DEFAULTS) end private diff --git a/lib/gitlab/github_import/project_creator.rb b/lib/gitlab/github_import/project_creator.rb index 3f635be22ba..a55adc9b1c8 100644 --- a/lib/gitlab/github_import/project_creator.rb +++ b/lib/gitlab/github_import/project_creator.rb @@ -1,6 +1,8 @@ module Gitlab module GithubImport class ProjectCreator + include Gitlab::CurrentSettings + attr_reader :repo, :name, :namespace, :current_user, :session_data, :type def initialize(repo, name, namespace, current_user, session_data, type: 'github') @@ -34,7 +36,7 @@ module Gitlab end def visibility_level - repo.private ? Gitlab::VisibilityLevel::PRIVATE : ApplicationSetting.current.default_project_visibility + repo.private ? Gitlab::VisibilityLevel::PRIVATE : current_application_settings.default_project_visibility end # diff --git a/spec/controllers/health_check_controller_spec.rb b/spec/controllers/health_check_controller_spec.rb index 56ecf2bb644..cfe18dd4b6c 100644 --- a/spec/controllers/health_check_controller_spec.rb +++ b/spec/controllers/health_check_controller_spec.rb @@ -1,10 +1,16 @@ require 'spec_helper' describe HealthCheckController do + include StubENV + let(:token) { current_application_settings.health_check_access_token } let(:json_response) { JSON.parse(response.body) } let(:xml_response) { Hash.from_xml(response.body)['hash'] } + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') + end + describe 'GET #index' do context 'when services are up but NO access token' do it 'returns a not found page' do diff --git a/spec/features/admin/admin_disables_git_access_protocol_spec.rb b/spec/features/admin/admin_disables_git_access_protocol_spec.rb index 66044b44495..e8e080ce3e2 100644 --- a/spec/features/admin/admin_disables_git_access_protocol_spec.rb +++ b/spec/features/admin/admin_disables_git_access_protocol_spec.rb @@ -1,10 +1,13 @@ require 'rails_helper' feature 'Admin disables Git access protocol', feature: true do + include StubENV + let(:project) { create(:empty_project, :empty_repo) } let(:admin) { create(:admin) } background do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') login_as(admin) end diff --git a/spec/features/admin/admin_health_check_spec.rb b/spec/features/admin/admin_health_check_spec.rb index dec2dedf2b5..f7e49a56deb 100644 --- a/spec/features/admin/admin_health_check_spec.rb +++ b/spec/features/admin/admin_health_check_spec.rb @@ -1,9 +1,11 @@ require 'spec_helper' feature "Admin Health Check", feature: true do + include StubENV include WaitForAjax before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') login_as :admin end @@ -12,11 +14,12 @@ feature "Admin Health Check", feature: true do visit admin_health_check_path end - it { page.has_text? 'Health Check' } - it { page.has_text? 'Health information can be retrieved' } - it 'has a health check access token' do + page.has_text? 'Health Check' + page.has_text? 'Health information can be retrieved' + token = current_application_settings.health_check_access_token + expect(page).to have_content("Access token is #{token}") expect(page).to have_selector('#health-check-token', text: token) end diff --git a/spec/features/admin/admin_runners_spec.rb b/spec/features/admin/admin_runners_spec.rb index d92c66b689d..f05fbe3d062 100644 --- a/spec/features/admin/admin_runners_spec.rb +++ b/spec/features/admin/admin_runners_spec.rb @@ -1,7 +1,10 @@ require 'spec_helper' describe "Admin Runners" do + include StubENV + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') login_as :admin end diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index 47fa2f14307..de42ab81fac 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -1,7 +1,10 @@ require 'spec_helper' feature 'Admin updates settings', feature: true do - before(:each) do + include StubENV + + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') login_as :admin visit admin_application_settings_path end diff --git a/spec/features/admin/admin_uses_repository_checks_spec.rb b/spec/features/admin/admin_uses_repository_checks_spec.rb index 661fb761809..855247de2ea 100644 --- a/spec/features/admin/admin_uses_repository_checks_spec.rb +++ b/spec/features/admin/admin_uses_repository_checks_spec.rb @@ -1,7 +1,12 @@ require 'rails_helper' feature 'Admin uses repository checks', feature: true do - before { login_as :admin } + include StubENV + + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') + login_as :admin + end scenario 'to trigger a single check' do project = create(:empty_project) @@ -29,7 +34,7 @@ feature 'Admin uses repository checks', feature: true do scenario 'to clear all repository checks', js: true do visit admin_application_settings_path - + expect(RepositoryCheck::ClearWorker).to receive(:perform_async) click_link 'Clear all repository checks' diff --git a/spec/lib/gitlab/current_settings_spec.rb b/spec/lib/gitlab/current_settings_spec.rb index 004341ffd02..b01c4805a34 100644 --- a/spec/lib/gitlab/current_settings_spec.rb +++ b/spec/lib/gitlab/current_settings_spec.rb @@ -1,36 +1,64 @@ require 'spec_helper' describe Gitlab::CurrentSettings do + include StubENV + + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') + end + describe '#current_application_settings' do - it 'attempts to use cached values first' do - allow_any_instance_of(Gitlab::CurrentSettings).to receive(:connect_to_db?).and_return(true) - expect(ApplicationSetting).to receive(:current).and_return(::ApplicationSetting.create_from_defaults) - expect(ApplicationSetting).not_to receive(:last) + context 'with DB available' do + before do + allow_any_instance_of(Gitlab::CurrentSettings).to receive(:connect_to_db?).and_return(true) + end - expect(current_application_settings).to be_a(ApplicationSetting) + it 'attempts to use cached values first' do + expect(ApplicationSetting).to receive(:current) + expect(ApplicationSetting).not_to receive(:last) + + expect(current_application_settings).to be_a(ApplicationSetting) + end + + it 'falls back to DB if Redis returns an empty value' do + expect(ApplicationSetting).to receive(:last).and_call_original + + expect(current_application_settings).to be_a(ApplicationSetting) + end + + it 'falls back to DB if Redis fails' do + expect(ApplicationSetting).to receive(:current).and_raise(::Redis::BaseError) + expect(ApplicationSetting).to receive(:last).and_call_original + + expect(current_application_settings).to be_a(ApplicationSetting) + end end - it 'does not attempt to connect to DB or Redis' do - allow_any_instance_of(Gitlab::CurrentSettings).to receive(:connect_to_db?).and_return(false) - expect(ApplicationSetting).not_to receive(:current) - expect(ApplicationSetting).not_to receive(:last) + context 'with DB unavailable' do + before do + allow_any_instance_of(Gitlab::CurrentSettings).to receive(:connect_to_db?).and_return(false) + end - expect(current_application_settings).to eq fake_application_settings + it 'returns an in-memory ApplicationSetting object' do + expect(ApplicationSetting).not_to receive(:current) + expect(ApplicationSetting).not_to receive(:last) + + expect(current_application_settings).to be_a(OpenStruct) + end end - it 'falls back to DB if Redis returns an empty value' do - allow_any_instance_of(Gitlab::CurrentSettings).to receive(:connect_to_db?).and_return(true) - expect(ApplicationSetting).to receive(:last).and_call_original + context 'when ENV["IN_MEMORY_APPLICATION_SETTINGS"] is true' do + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'true') + end - expect(current_application_settings).to be_a(ApplicationSetting) - end + it 'returns an in-memory ApplicationSetting object' do + expect(ApplicationSetting).not_to receive(:current) + expect(ApplicationSetting).not_to receive(:last) - it 'falls back to DB if Redis fails' do - allow_any_instance_of(Gitlab::CurrentSettings).to receive(:connect_to_db?).and_return(true) - expect(ApplicationSetting).to receive(:current).and_raise(::Redis::BaseError) - expect(ApplicationSetting).to receive(:last).and_call_original - - expect(current_application_settings).to be_a(ApplicationSetting) + expect(current_application_settings).to be_a(ApplicationSetting) + expect(current_application_settings).not_to be_persisted + end end end end diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index 35644bd8cc9..c0a01d37990 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -337,8 +337,7 @@ describe API::Internal, api: true do context 'ssh access has been disabled' do before do - settings = ::ApplicationSetting.create_from_defaults - settings.update_attribute(:enabled_git_access_protocol, 'http') + stub_application_setting(enabled_git_access_protocol: 'http') end it 'rejects the SSH push' do @@ -360,8 +359,7 @@ describe API::Internal, api: true do context 'http access has been disabled' do before do - settings = ::ApplicationSetting.create_from_defaults - settings.update_attribute(:enabled_git_access_protocol, 'ssh') + stub_application_setting(enabled_git_access_protocol: 'ssh') end it 'rejects the HTTP push' do @@ -383,8 +381,7 @@ describe API::Internal, api: true do context 'web actions are always allowed' do it 'allows WEB push' do - settings = ::ApplicationSetting.create_from_defaults - settings.update_attribute(:enabled_git_access_protocol, 'ssh') + stub_application_setting(enabled_git_access_protocol: 'ssh') project.team << [user, :developer] push(key, project, 'web') diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 6ee3307512d..f78899134d5 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -2,6 +2,7 @@ require './spec/simplecov_env' SimpleCovEnv.start! ENV["RAILS_ENV"] ||= 'test' +ENV["IN_MEMORY_APPLICATION_SETTINGS"] = 'true' require File.expand_path("../../config/environment", __FILE__) require 'rspec/rails' From db0f9ef4a871ec77f347ce2437bd7e8adcb42986 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 16 Jan 2017 14:39:38 -0500 Subject: [PATCH 018/130] Make cycle_analytics_events_spec.rb side-effect free MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- .../projects/cycle_analytics_events_spec.rb | 35 ++++++++----------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/spec/requests/projects/cycle_analytics_events_spec.rb b/spec/requests/projects/cycle_analytics_events_spec.rb index e0368e6001f..72978846e93 100644 --- a/spec/requests/projects/cycle_analytics_events_spec.rb +++ b/spec/requests/projects/cycle_analytics_events_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe 'cycle analytics events' do + include ApiHelpers + let(:user) { create(:user) } let(:project) { create(:project, public_builds: false) } let(:issue) { create(:issue, project: project, created_at: 2.days.ago) } @@ -20,19 +22,19 @@ describe 'cycle analytics events' do it 'lists the issue events' do get namespace_project_cycle_analytics_issue_path(project.namespace, project, format: :json) + first_issue_iid = project.issues.sort(:created_desc).pluck(:iid).first.to_s + expect(json_response['events']).not_to be_empty - - first_issue_iid = Issue.order(created_at: :desc).pluck(:iid).first.to_s - expect(json_response['events'].first['iid']).to eq(first_issue_iid) end it 'lists the plan events' do get namespace_project_cycle_analytics_plan_path(project.namespace, project, format: :json) - expect(json_response['events']).not_to be_empty + first_mr_short_sha = project.merge_requests.sort(:created_asc).first.commits.first.short_id - expect(json_response['events'].first['short_sha']).to eq(MergeRequest.last.commits.first.short_id) + expect(json_response['events']).not_to be_empty + expect(json_response['events'].first['short_sha']).to eq(first_mr_short_sha) end it 'lists the code events' do @@ -40,7 +42,7 @@ describe 'cycle analytics events' do expect(json_response['events']).not_to be_empty - first_mr_iid = project.merge_requests.order(id: :desc).pluck(:iid).first.to_s + first_mr_iid = project.merge_requests.sort(:created_desc).pluck(:iid).first.to_s expect(json_response['events'].first['iid']).to eq(first_mr_iid) end @@ -49,17 +51,15 @@ describe 'cycle analytics events' do get namespace_project_cycle_analytics_test_path(project.namespace, project, format: :json) expect(json_response['events']).not_to be_empty - expect(json_response['events'].first['date']).not_to be_empty end it 'lists the review events' do get namespace_project_cycle_analytics_review_path(project.namespace, project, format: :json) + first_mr_iid = project.merge_requests.sort(:created_desc).pluck(:iid).first.to_s + expect(json_response['events']).not_to be_empty - - first_mr_iid = MergeRequest.order(created_at: :desc).pluck(:iid).first.to_s - expect(json_response['events'].first['iid']).to eq(first_mr_iid) end @@ -67,35 +67,32 @@ describe 'cycle analytics events' do get namespace_project_cycle_analytics_staging_path(project.namespace, project, format: :json) expect(json_response['events']).not_to be_empty - expect(json_response['events'].first['date']).not_to be_empty end it 'lists the production events' do get namespace_project_cycle_analytics_production_path(project.namespace, project, format: :json) + first_issue_iid = project.issues.sort(:created_desc).pluck(:iid).first.to_s + expect(json_response['events']).not_to be_empty - - first_issue_iid = Issue.order(created_at: :desc).pluck(:iid).first.to_s - expect(json_response['events'].first['iid']).to eq(first_issue_iid) end context 'specific branch' do it 'lists the test events' do - branch = MergeRequest.first.source_branch + branch = project.merge_requests.first.source_branch get namespace_project_cycle_analytics_test_path(project.namespace, project, format: :json, branch: branch) expect(json_response['events']).not_to be_empty - expect(json_response['events'].first['date']).not_to be_empty end end context 'with private project and builds' do before do - ProjectMember.first.update(access_level: Gitlab::Access::GUEST) + project.members.first.update(access_level: Gitlab::Access::GUEST) end it 'does not list the test events' do @@ -118,10 +115,6 @@ describe 'cycle analytics events' do end end - def json_response - JSON.parse(response.body) - end - def create_cycle milestone = create(:milestone, project: project) issue.update(milestone: milestone) From cff9c16be724398e3d6e7170cc9f5e39cfd8cdb7 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 18 Jan 2017 11:07:12 +0100 Subject: [PATCH 019/130] 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 2a97e8bae4a..fab8497ec7d 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -128,16 +128,21 @@ module Ci 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 d4b6ff910aa..ca76d82f358 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -8,10 +8,11 @@ module Ci 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 @@ module Ci 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 ee3b17b8bf1..7f557b25ccb 100644 --- a/spec/factories/ci/stages.rb +++ b/spec/factories/ci/stages.rb @@ -3,11 +3,12 @@ FactoryGirl.define do 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 3d387d52c8e..cca0cb1e3b0 100644 --- a/spec/models/ci/stage_spec.rb +++ b/spec/models/ci/stage_spec.rb @@ -168,13 +168,23 @@ describe Ci::Stage, models: true do 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 From e66b0414cb0a81565937391252e841c777bf270b Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 18 Jan 2017 11:25:36 +0100 Subject: [PATCH 020/130] Improve readability of specs for pipeline stages --- spec/models/ci/pipeline_spec.rb | 101 ++++++++++++++++++++------------ 1 file changed, 63 insertions(+), 38 deletions(-) diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index d1aee27057a..2bdd611aeed 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -122,55 +122,80 @@ describe Ci::Pipeline, models: true do 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') + 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 } + 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 - - it 'returns a valid number of stages' do - expect(pipeline.stages_count).to eq(3) - end - - it 'returns a valid names of stages' do - expect(pipeline.stages_name).to eq(['build', 'test', 'deploy']) - end - - context 'stages with statuses' do - let(:statuses) do - subject.map do |stage| - [stage.name, stage.status] + context 'stages list' do + it 'returns ordered list of stages' do + expect(subject.map(&:name)).to eq(%w[build test deploy]) end end - it 'returns list of stages with statuses' do - expect(statuses).to eq([['build', 'failed'], - ['test', 'success'], - ['deploy', 'running'] - ]) - end - - context 'when build is retried' do - before do - create(:commit_status, pipeline: pipeline, stage: 'build', name: 'mac', stage_idx: 0, status: 'success') + context 'stages with statuses' do + let(:statuses) do + subject.map { |stage| [stage.name, stage.status] } end - it 'ignores the previous state' do - expect(statuses).to eq([['build', 'success'], + it 'returns list of stages with correct statuses' do + expect(statuses).to eq([['build', 'failed'], ['test', 'success'], - ['deploy', 'running'] - ]) + ['deploy', 'running']]) end + + 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 + + describe '#stages_count' do + it 'returns a valid number of stages' do + expect(pipeline.stages_count).to eq(3) + end + 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 From 3bc0525ad90aa84ff864a0bc9c43e18ec5d5cd3d Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 18 Jan 2017 11:30:28 +0100 Subject: [PATCH 021/130] 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 3da5ae4fc01..4d7c71ed469 100644 --- a/lib/gitlab/ci/status/factory.rb +++ b/lib/gitlab/ci/status/factory.rb @@ -12,9 +12,7 @@ module Gitlab 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 @@ module Gitlab .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) From 73fcfb296c90746f868ec11a19477a16039ef9a5 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 18 Jan 2017 11:34:55 +0100 Subject: [PATCH 022/130] 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 90432fc4050..431c0354969 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 4d7c71ed469..15836c699c7 100644 --- a/lib/gitlab/ci/status/factory.rb +++ b/lib/gitlab/ci/status/factory.rb @@ -5,7 +5,7 @@ module Gitlab 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 4d0f51fe82a..dbfe3cd2d36 100644 --- a/spec/models/concerns/has_status_spec.rb +++ b/spec/models/concerns/has_status_spec.rb @@ -219,4 +219,10 @@ describe HasStatus do end end end + + describe '::DEFAULT_STATUS' do + it 'is a status created' do + expect(described_class::DEFAULT_STATUS).to eq 'created' + end + end end From 35442766cf1041d2bb100ff73f58f92347649027 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 18 Jan 2017 14:13:01 +0100 Subject: [PATCH 023/130] do not map usersat all unless admin --- lib/gitlab/import_export/members_mapper.rb | 4 ++-- .../lib/gitlab/import_export/members_mapper_spec.rb | 13 ++++++++++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/import_export/members_mapper.rb b/lib/gitlab/import_export/members_mapper.rb index b790733f4a7..1dab7c37d25 100644 --- a/lib/gitlab/import_export/members_mapper.rb +++ b/lib/gitlab/import_export/members_mapper.rb @@ -4,7 +4,7 @@ module Gitlab attr_reader :missing_author_ids def initialize(exported_members:, user:, project:) - @exported_members = exported_members + @exported_members = user.admin? ? exported_members : [] @user = user @project = project @missing_author_ids = [] @@ -64,7 +64,7 @@ module Gitlab end def find_project_user_query(member) - user_arel[:username].eq(member['user']['username']).or(user_arel[:email].eq(member['user']['email'])) + user_arel[:email].eq(member['user']['email']).or(user_arel[:username].eq(member['user']['username'])) end def user_arel diff --git a/spec/lib/gitlab/import_export/members_mapper_spec.rb b/spec/lib/gitlab/import_export/members_mapper_spec.rb index 1cb02f8e318..95666a7cc02 100644 --- a/spec/lib/gitlab/import_export/members_mapper_spec.rb +++ b/spec/lib/gitlab/import_export/members_mapper_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Gitlab::ImportExport::MembersMapper, services: true do describe 'map members' do - let(:user) { create(:user, authorized_projects_populated: true) } + let(:user) { create(:admin, authorized_projects_populated: true) } let(:project) { create(:project, :public, name: 'searchable_project') } let(:user2) { create(:user, authorized_projects_populated: true) } let(:exported_user_id) { 99 } @@ -74,5 +74,16 @@ describe Gitlab::ImportExport::MembersMapper, services: true do expect(user.authorized_project?(project)).to be true expect(user2.authorized_project?(project)).to be true end + + context 'user is not admin' do + let(:user) { create(:user, authorized_projects_populated: true) } + it 'does not map a project member' do + expect(members_mapper.map[exported_user_id]).to eq(user.id) + end + + it 'defaults to importer project member if it does not exist' do + expect(members_mapper.map[-1]).to eq(user.id) + end + end end end From b3bb8dc46faf448d02e917586e7666143d7ab968 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 18 Jan 2017 15:46:54 +0100 Subject: [PATCH 024/130] added spec replicating the problem --- lib/gitlab/import_export/relation_factory.rb | 2 +- .../import_export/members_mapper_spec.rb | 3 +- .../import_export/relation_factory_spec.rb | 58 ++++++++++++++++++- 3 files changed, 60 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/import_export/relation_factory.rb b/lib/gitlab/import_export/relation_factory.rb index 7a649f28340..a5f6fbbcfd3 100644 --- a/lib/gitlab/import_export/relation_factory.rb +++ b/lib/gitlab/import_export/relation_factory.rb @@ -14,7 +14,7 @@ module Gitlab priorities: :label_priorities, label: :project_label }.freeze - USER_REFERENCES = %w[author_id assignee_id updated_by_id user_id created_by_id merge_user_id].freeze + USER_REFERENCES = %w[author_id assignee_id updated_by_id user_id created_by_id merge_user_id resolved_by_id].freeze PROJECT_REFERENCES = %w[project_id source_project_id gl_project_id target_project_id].freeze diff --git a/spec/lib/gitlab/import_export/members_mapper_spec.rb b/spec/lib/gitlab/import_export/members_mapper_spec.rb index 95666a7cc02..af3a0ab2b45 100644 --- a/spec/lib/gitlab/import_export/members_mapper_spec.rb +++ b/spec/lib/gitlab/import_export/members_mapper_spec.rb @@ -75,8 +75,9 @@ describe Gitlab::ImportExport::MembersMapper, services: true do expect(user2.authorized_project?(project)).to be true end - context 'user is not admin' do + context 'user is not an admin' do let(:user) { create(:user, authorized_projects_populated: true) } + it 'does not map a project member' do expect(members_mapper.map[exported_user_id]).to eq(user.id) end diff --git a/spec/lib/gitlab/import_export/relation_factory_spec.rb b/spec/lib/gitlab/import_export/relation_factory_spec.rb index 3aa492a8ab1..4604e88b295 100644 --- a/spec/lib/gitlab/import_export/relation_factory_spec.rb +++ b/spec/lib/gitlab/import_export/relation_factory_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Gitlab::ImportExport::RelationFactory, lib: true do let(:project) { create(:empty_project) } let(:members_mapper) { double('members_mapper').as_null_object } - let(:user) { create(:user) } + let(:user) { create(:admin) } let(:created_object) do described_class.create(relation_sym: relation_sym, relation_hash: relation_hash, @@ -122,4 +122,60 @@ describe Gitlab::ImportExport::RelationFactory, lib: true do expect(created_object.values).not_to include(99) end end + + context 'Notes user references' do + let(:relation_sym) { :notes } + let(:new_user) { create(:user) } + let(:exported_member) do + { + "id" => 999, + "access_level" => 30, + "source_id" => 1, + "source_type" => "Project", + "user_id" => 3, + "notification_level" => 3, + "created_at" => "2016-11-18T09:29:42.634Z", + "updated_at" => "2016-11-18T09:29:42.634Z", + "user" => { + "id" => new_user.id, + "email" => new_user.email, + "username" => new_user.username + } + } + end + + let(:relation_hash) do + { + "id" => 4947, + "note" => "merged", + "noteable_type" => "MergeRequest", + "author_id" => 999, + "created_at" => "2016-11-18T09:29:42.634Z", + "updated_at" => "2016-11-18T09:29:42.634Z", + "project_id" => 1, + "attachment" => { + "url" => nil + }, + "noteable_id" => 377, + "system" => true, + "author" => { + "name" => "Administrator" + }, + "events" => [ + + ] + } + end + + let(:members_mapper) do + Gitlab::ImportExport::MembersMapper.new( + exported_members: [exported_member], + user: user, + project: project) + end + + it 'maps the right author to the imported note' do + expect(created_object.author).to eq(new_user) + end + end end From 17c099161ee582e627a531bda1d84574d6a8c0f7 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 18 Jan 2017 17:40:24 +0100 Subject: [PATCH 025/130] fix and refactor note user mapping --- lib/gitlab/import_export/members_mapper.rb | 2 - lib/gitlab/import_export/relation_factory.rb | 10 ++--- .../import_export/members_mapper_spec.rb | 43 ++++++++++++++++--- .../import_export/relation_factory_spec.rb | 4 +- 4 files changed, 41 insertions(+), 18 deletions(-) diff --git a/lib/gitlab/import_export/members_mapper.rb b/lib/gitlab/import_export/members_mapper.rb index 1dab7c37d25..ac7604d2461 100644 --- a/lib/gitlab/import_export/members_mapper.rb +++ b/lib/gitlab/import_export/members_mapper.rb @@ -7,7 +7,6 @@ module Gitlab @exported_members = user.admin? ? exported_members : [] @user = user @project = project - @missing_author_ids = [] # This needs to run first, as second call would be from #map # which means project members already exist. @@ -39,7 +38,6 @@ module Gitlab def missing_keys_tracking_hash Hash.new do |_, key| - @missing_author_ids << key default_user_id end end diff --git a/lib/gitlab/import_export/relation_factory.rb b/lib/gitlab/import_export/relation_factory.rb index a5f6fbbcfd3..e5f9aa39190 100644 --- a/lib/gitlab/import_export/relation_factory.rb +++ b/lib/gitlab/import_export/relation_factory.rb @@ -80,17 +80,13 @@ module Gitlab # is left. def set_note_author old_author_id = @relation_hash['author_id'] - - # Users with admin access can map users - @relation_hash['author_id'] = admin_user? ? @members_mapper.map[old_author_id] : @members_mapper.default_user_id - author = @relation_hash.delete('author') - update_note_for_missing_author(author['name']) if missing_author?(old_author_id) + update_note_for_missing_author(author['name']) unless has_author?(old_author_id) end - def missing_author?(old_author_id) - !admin_user? || @members_mapper.missing_author_ids.include?(old_author_id) + def has_author?(old_author_id) + admin_user? && !@members_mapper.map.keys.include?(old_author_id) end def missing_author_note(updated_at, author_name) diff --git a/spec/lib/gitlab/import_export/members_mapper_spec.rb b/spec/lib/gitlab/import_export/members_mapper_spec.rb index af3a0ab2b45..39e6dad5abe 100644 --- a/spec/lib/gitlab/import_export/members_mapper_spec.rb +++ b/spec/lib/gitlab/import_export/members_mapper_spec.rb @@ -24,7 +24,7 @@ describe Gitlab::ImportExport::MembersMapper, services: true do { "id" => exported_user_id, "email" => user2.email, - "username" => user2.username + "username" => 'test' } }, { @@ -48,6 +48,12 @@ describe Gitlab::ImportExport::MembersMapper, services: true do exported_members: exported_members, user: user, project: project) end + it 'includes the exported user ID in the map' do + members_mapper.map[-1] + + expect(members_mapper.map.keys).to include(exported_user_id) + end + it 'maps a project member' do expect(members_mapper.map[exported_user_id]).to eq(user2.id) end @@ -56,12 +62,6 @@ describe Gitlab::ImportExport::MembersMapper, services: true do expect(members_mapper.map[-1]).to eq(user.id) end - it 'updates missing author IDs on missing project member' do - members_mapper.map[-1] - - expect(members_mapper.missing_author_ids.first).to eq(-1) - end - it 'has invited members with no user' do members_mapper.map @@ -86,5 +86,34 @@ describe Gitlab::ImportExport::MembersMapper, services: true do expect(members_mapper.map[-1]).to eq(user.id) end end + + context 'chooses the one with an email first' do + before do + exported_members << { + "id" => 2, + "access_level" => 40, + "source_id" => 14, + "source_type" => "Project", + "user_id" => 19, + "notification_level" => 3, + "created_at" => "2016-03-11T10:21:44.822Z", + "updated_at" => "2016-03-11T10:21:44.822Z", + "created_by_id" => nil, + "invite_email" => nil, + "invite_token" => nil, + "invite_accepted_at" => nil, + "user" => + { + "id" => exported_user_id, + "email" => 'test@email.com', + "username" => user2.username + } + } + end + + it 'maps the project member that has a matching email first' do + expect(members_mapper.map[exported_user_id]).to eq(user2.id) + end + end end end diff --git a/spec/lib/gitlab/import_export/relation_factory_spec.rb b/spec/lib/gitlab/import_export/relation_factory_spec.rb index 4604e88b295..db0084d6823 100644 --- a/spec/lib/gitlab/import_export/relation_factory_spec.rb +++ b/spec/lib/gitlab/import_export/relation_factory_spec.rb @@ -128,7 +128,7 @@ describe Gitlab::ImportExport::RelationFactory, lib: true do let(:new_user) { create(:user) } let(:exported_member) do { - "id" => 999, + "id" => 111, "access_level" => 30, "source_id" => 1, "source_type" => "Project", @@ -137,7 +137,7 @@ describe Gitlab::ImportExport::RelationFactory, lib: true do "created_at" => "2016-11-18T09:29:42.634Z", "updated_at" => "2016-11-18T09:29:42.634Z", "user" => { - "id" => new_user.id, + "id" => 999, "email" => new_user.email, "username" => new_user.username } From 9ba288dd81690a20432291fde3fb91ffaadfd114 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 19 Jan 2017 09:01:09 +0100 Subject: [PATCH 026/130] add missing changelog --- changelogs/unreleased/fix-import-users.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/fix-import-users.yml diff --git a/changelogs/unreleased/fix-import-users.yml b/changelogs/unreleased/fix-import-users.yml new file mode 100644 index 00000000000..bb483bb9417 --- /dev/null +++ b/changelogs/unreleased/fix-import-users.yml @@ -0,0 +1,4 @@ +--- +title: Fix import/export wrong user mapping +merge_request: +author: From 63e8dc8a76184ff76feb9c3f4c21d3180edd218c Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Wed, 21 Dec 2016 20:09:44 +0530 Subject: [PATCH 027/130] Add documentation around OAuth/Personal Access Token scopes. --- doc/api/README.md | 8 ++++++++ doc/integration/oauth_provider.md | 8 +++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/doc/api/README.md b/doc/api/README.md index f65b934b9db..20f28e8d30e 100644 --- a/doc/api/README.md +++ b/doc/api/README.md @@ -104,6 +104,13 @@ that needs access to the GitLab API. Once you have your token, pass it to the API using either the `private_token` parameter or the `PRIVATE-TOKEN` header. +> [Introduced][ce-5951] in GitLab 8.15. + +Personal Access Tokens can be created with one or more scopes that allow various actions +that a given token can perform. Although there are only two scopes available at the +moment – `read_user` and `api` – the groundwork has been laid to add more scopes easily. + +At any time you can revoke any personal access token by just clicking **Revoke**. ### Session Cookie @@ -380,3 +387,4 @@ programming languages. Visit the [GitLab website] for a complete list. [GitLab website]: https://about.gitlab.com/applications/#api-clients "Clients using the GitLab API" [lib-api-url]: https://gitlab.com/gitlab-org/gitlab-ce/tree/master/lib/api/api.rb [ce-3749]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/3749 +[ce-5951]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5951 diff --git a/doc/integration/oauth_provider.md b/doc/integration/oauth_provider.md index 0c53584d201..af8a1c4e5ed 100644 --- a/doc/integration/oauth_provider.md +++ b/doc/integration/oauth_provider.md @@ -74,8 +74,10 @@ in the **Authorized applications** section under **Profile Settings > Applicatio --- -As you can see, the default scope `api` is used, which is the only scope that -GitLab supports so far. At any time you can revoke any access by just clicking -**Revoke**. +GitLab's OAuth applications support scopes, which allow various actions that any given +application can perform. Although there are only two scopes available at the +moment – `read_user` and `api` – the groundwork has been laid to add more scopes easily. + +At any time you can revoke any access by just clicking **Revoke**. [oauth]: http://oauth.net/2/ "OAuth website" From e8a9682bc98bd5e50ca52b39891cf3f7945b2300 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 19 Jan 2017 09:44:20 +0100 Subject: [PATCH 028/130] fix typo --- lib/gitlab/import_export/relation_factory.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/import_export/relation_factory.rb b/lib/gitlab/import_export/relation_factory.rb index e5f9aa39190..19e43cce768 100644 --- a/lib/gitlab/import_export/relation_factory.rb +++ b/lib/gitlab/import_export/relation_factory.rb @@ -86,7 +86,7 @@ module Gitlab end def has_author?(old_author_id) - admin_user? && !@members_mapper.map.keys.include?(old_author_id) + admin_user? && @members_mapper.map.keys.include?(old_author_id) end def missing_author_note(updated_at, author_name) From 86217866fda66cb770ee8c9a540bf535a980eb36 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 19 Jan 2017 18:49:14 +0100 Subject: [PATCH 029/130] 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 ca76d82f358..ca74c91b062 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -46,7 +46,11 @@ module Ci 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 cca0cb1e3b0..c4a9743a4e2 100644 --- a/spec/models/ci/stage_spec.rb +++ b/spec/models/ci/stage_spec.rb @@ -169,10 +169,22 @@ describe Ci::Stage, models: true do 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 @@ describe Ci::Stage, models: true do 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 @@ describe Ci::Stage, models: true do 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 From 5db587a1adb6c51a2e3e5ab44ef35779f9a6917d Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 6 Jan 2017 11:58:51 +0000 Subject: [PATCH 030/130] Responsive title in diffs inline, side by side, with and without sidebar Adds MR ID to CHANGELOG entry --- app/assets/stylesheets/pages/diff.scss | 72 +++++++++++++++++++ .../projects/diffs/_file_header.html.haml | 6 +- .../unreleased/25709-diff-file-overflow.yml | 4 ++ 3 files changed, 79 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/25709-diff-file-overflow.yml diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index 96ba7c40634..d7bb029cac6 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -44,6 +44,15 @@ .diff-toggle-caret { padding-right: 6px; } + + .file-title-name { + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; + display: inline-block; + vertical-align: bottom; + max-width: 450px; + } } .diff-content { @@ -480,3 +489,66 @@ } } } + +/** + * Diff file title + */ +.file-holder[data-view="parallel"] .file-title-name, +.file-holder[data-view="inline"] .file-title-name { + @media (max-width: $screen-xs) { + max-width: 140px; + } + + @media (min-width: $screen-xs) and (max-width: $screen-xs-max) { + max-width: 250px; + } + + @media (min-width: $screen-sm) and (max-width: $screen-sm-max) { + max-width: 250px; + } + + @media (min-width: $screen-md) and (max-width: $screen-md-max) { + max-width: 480px; + } +} + +.file-holder[data-view="parallel"] .file-title-name { + @media (min-width: $screen-lg) { + max-width: 760px; + } +} + +.file-holder[data-view="inline"] .file-title-name { + @media (min-width: $screen-lg) { + max-width: 530px; + } +} + +.right-sidebar-expanded { + .file-holder[data-view="parallel"] .file-title-name, + .file-holder[data-view="inline"] .file-title-name { + @media (min-width: $screen-sm) and (max-width: $screen-sm-max) { + max-width: 250px; + } + + @media (min-width: $screen-md) and (max-width: $screen-md-max) { + max-width: 250px; + } + + @media (min-width: $screen-lg) { + max-width: 460px; + } + } + + .file-holder[data-view="parallel"] .file-title-name { + @media (min-width: $screen-xs) and (max-width: $screen-xs-max) { + max-width: 180px; + } + } + + .file-holder[data-view="inline"] .file-title-name { + @media (min-width: $screen-xs) and (max-width: $screen-xs-max) { + max-width: 100px; + } + } +} diff --git a/app/views/projects/diffs/_file_header.html.haml b/app/views/projects/diffs/_file_header.html.haml index 90c9a0c6c2b..7fe450381c5 100644 --- a/app/views/projects/diffs/_file_header.html.haml +++ b/app/views/projects/diffs/_file_header.html.haml @@ -10,13 +10,13 @@ - if diff_file.renamed_file - old_path, new_path = mark_inline_diffs(diff_file.old_path, diff_file.new_path) - %strong + %strong.file-title-name = old_path → - %strong + %strong.file-title-name = new_path - else - %strong + %strong.file-title-name = diff_file.new_path - if diff_file.deleted_file deleted diff --git a/changelogs/unreleased/25709-diff-file-overflow.yml b/changelogs/unreleased/25709-diff-file-overflow.yml new file mode 100644 index 00000000000..7d1b2b36ab8 --- /dev/null +++ b/changelogs/unreleased/25709-diff-file-overflow.yml @@ -0,0 +1,4 @@ +--- +title: Responsive title in diffs inline, side by side, with and without sidebar +merge_request: 8475 +author: From 815c0910968fb4c0f0c787af5e7733c71284a274 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 12 Jan 2017 11:15:42 -0500 Subject: [PATCH 031/130] Adds tooltips --- app/assets/stylesheets/pages/diff.scss | 4 ++++ app/views/projects/diffs/_file_header.html.haml | 6 +++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index d7bb029cac6..d614fa45ad3 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -37,6 +37,10 @@ .file-title { cursor: pointer; + a:hover { + text-decoration: none; + } + &:hover { background-color: $gray-normal; } diff --git a/app/views/projects/diffs/_file_header.html.haml b/app/views/projects/diffs/_file_header.html.haml index 7fe450381c5..59b91c057a8 100644 --- a/app/views/projects/diffs/_file_header.html.haml +++ b/app/views/projects/diffs/_file_header.html.haml @@ -10,13 +10,13 @@ - if diff_file.renamed_file - old_path, new_path = mark_inline_diffs(diff_file.old_path, diff_file.new_path) - %strong.file-title-name + %strong.file-title-name.has-tooltip{data: { title: old_path } } = old_path → - %strong.file-title-name + %strong.file-title-name.has-tooltip{data: { title: new_path } } = new_path - else - %strong.file-title-name + %strong.file-title-name.has-tooltip{data: { title: diff_file.new_path } } = diff_file.new_path - if diff_file.deleted_file deleted From 28b4407bded6dd526bfafa5e498b4ac109fec7f7 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 12 Jan 2017 12:03:24 -0500 Subject: [PATCH 032/130] Fix haml linter errors --- app/views/projects/diffs/_file_header.html.haml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/views/projects/diffs/_file_header.html.haml b/app/views/projects/diffs/_file_header.html.haml index 59b91c057a8..1e5ac4ed38c 100644 --- a/app/views/projects/diffs/_file_header.html.haml +++ b/app/views/projects/diffs/_file_header.html.haml @@ -10,13 +10,13 @@ - if diff_file.renamed_file - old_path, new_path = mark_inline_diffs(diff_file.old_path, diff_file.new_path) - %strong.file-title-name.has-tooltip{data: { title: old_path } } + %strong.file-title-name.has-tooltip{ data: { title: old_path } } = old_path → - %strong.file-title-name.has-tooltip{data: { title: new_path } } + %strong.file-title-name.has-tooltip{ data: { title: new_path } } = new_path - else - %strong.file-title-name.has-tooltip{data: { title: diff_file.new_path } } + %strong.file-title-name.has-tooltip{ data: { title: diff_file.new_path } } = diff_file.new_path - if diff_file.deleted_file deleted From 2bb2544ecaf37a02e4055194e90405dc1ffca094 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Wed, 18 Jan 2017 18:56:45 +0100 Subject: [PATCH 033/130] Use flexbox instead of media queries Move CSS to correct file --- app/assets/stylesheets/framework/files.scss | 34 ++++++++++ app/assets/stylesheets/pages/diff.scss | 72 --------------------- app/views/projects/diffs/_file.html.haml | 4 +- 3 files changed, 36 insertions(+), 74 deletions(-) diff --git a/app/assets/stylesheets/framework/files.scss b/app/assets/stylesheets/framework/files.scss index c51912b4ac4..92170daa475 100644 --- a/app/assets/stylesheets/framework/files.scss +++ b/app/assets/stylesheets/framework/files.scss @@ -231,3 +231,37 @@ span.idiff { } } } + + +.file-title-flex-parent { + display: flex; + align-items: center; + background-color: $gray-light; + border-bottom: 1px solid $border-color; + padding: 10px $gl-padding; + margin: 0; + border-radius: 3px 3px 0 0; + + > a { + flex: 1; + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; + color: $gl-text-color; + } + + small { + margin: 0 10px 0 0; + } + + .file-actions-fixed { + white-space: nowrap; + + .btn { + padding: 0 10px; + font-size: 13px; + line-height: 28px; + display: inline-block; + } + } +} diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index d614fa45ad3..8b784c2a439 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -48,15 +48,6 @@ .diff-toggle-caret { padding-right: 6px; } - - .file-title-name { - white-space: nowrap; - overflow: hidden; - text-overflow: ellipsis; - display: inline-block; - vertical-align: bottom; - max-width: 450px; - } } .diff-content { @@ -493,66 +484,3 @@ } } } - -/** - * Diff file title - */ -.file-holder[data-view="parallel"] .file-title-name, -.file-holder[data-view="inline"] .file-title-name { - @media (max-width: $screen-xs) { - max-width: 140px; - } - - @media (min-width: $screen-xs) and (max-width: $screen-xs-max) { - max-width: 250px; - } - - @media (min-width: $screen-sm) and (max-width: $screen-sm-max) { - max-width: 250px; - } - - @media (min-width: $screen-md) and (max-width: $screen-md-max) { - max-width: 480px; - } -} - -.file-holder[data-view="parallel"] .file-title-name { - @media (min-width: $screen-lg) { - max-width: 760px; - } -} - -.file-holder[data-view="inline"] .file-title-name { - @media (min-width: $screen-lg) { - max-width: 530px; - } -} - -.right-sidebar-expanded { - .file-holder[data-view="parallel"] .file-title-name, - .file-holder[data-view="inline"] .file-title-name { - @media (min-width: $screen-sm) and (max-width: $screen-sm-max) { - max-width: 250px; - } - - @media (min-width: $screen-md) and (max-width: $screen-md-max) { - max-width: 250px; - } - - @media (min-width: $screen-lg) { - max-width: 460px; - } - } - - .file-holder[data-view="parallel"] .file-title-name { - @media (min-width: $screen-xs) and (max-width: $screen-xs-max) { - max-width: 180px; - } - } - - .file-holder[data-view="inline"] .file-title-name { - @media (min-width: $screen-xs) and (max-width: $screen-xs-max) { - max-width: 100px; - } - } -} diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml index c37a33bbcd5..430f7878839 100644 --- a/app/views/projects/diffs/_file.html.haml +++ b/app/views/projects/diffs/_file.html.haml @@ -1,9 +1,9 @@ .diff-file.file-holder{ id: file_hash, data: diff_file_html_data(project, diff_file.file_path, diff_commit.id) } - .file-title + .file-title-flex-parent = render "projects/diffs/file_header", diff_file: diff_file, blob: blob, diff_commit: diff_commit, project: project, url: "##{file_hash}" - unless diff_file.submodule? - .file-actions.hidden-xs + .file-actions-fixed.hidden-xs - if blob_text_viewable?(blob) = link_to '#', class: 'js-toggle-diff-comments btn active has-tooltip btn-file-option', title: "Toggle comments for this file", disabled: @diff_notes_disabled do = icon('comment') From 1446a03e79e7ecdf09e73854062f9ca6349aad9f Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Wed, 18 Jan 2017 16:48:51 -0600 Subject: [PATCH 034/130] Remove fixed class from file actions --- app/assets/stylesheets/framework/files.scss | 2 +- app/views/projects/diffs/_file.html.haml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/assets/stylesheets/framework/files.scss b/app/assets/stylesheets/framework/files.scss index 92170daa475..db1c8da06d0 100644 --- a/app/assets/stylesheets/framework/files.scss +++ b/app/assets/stylesheets/framework/files.scss @@ -254,7 +254,7 @@ span.idiff { margin: 0 10px 0 0; } - .file-actions-fixed { + .file-actions { white-space: nowrap; .btn { diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml index 430f7878839..7ab2eb1bce9 100644 --- a/app/views/projects/diffs/_file.html.haml +++ b/app/views/projects/diffs/_file.html.haml @@ -3,7 +3,7 @@ = render "projects/diffs/file_header", diff_file: diff_file, blob: blob, diff_commit: diff_commit, project: project, url: "##{file_hash}" - unless diff_file.submodule? - .file-actions-fixed.hidden-xs + .file-actions.hidden-xs - if blob_text_viewable?(blob) = link_to '#', class: 'js-toggle-diff-comments btn active has-tooltip btn-file-option', title: "Toggle comments for this file", disabled: @diff_notes_disabled do = icon('comment') From 2c72e0e7cd00148db2e27acae5a88f6a40c05245 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Thu, 19 Jan 2017 09:08:13 -0600 Subject: [PATCH 035/130] Fix participants margins to fit on one line --- app/assets/stylesheets/pages/issuable.scss | 4 ++++ changelogs/unreleased/participants-list.yml | 4 ++++ 2 files changed, 8 insertions(+) create mode 100644 changelogs/unreleased/participants-list.yml diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index 324c6cec96a..93cc5a8cf0a 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -377,6 +377,10 @@ display: inline-block; padding: 5px; + &:nth-of-type(7n) { + padding-right: 0; + } + .author_link { display: block; } diff --git a/changelogs/unreleased/participants-list.yml b/changelogs/unreleased/participants-list.yml new file mode 100644 index 00000000000..5265a2bc780 --- /dev/null +++ b/changelogs/unreleased/participants-list.yml @@ -0,0 +1,4 @@ +--- +title: Fix participants margins to fit on one line +merge_request: +author: From 0db574aaa74b8851dc9e65072944c30254ecad12 Mon Sep 17 00:00:00 2001 From: Regis Date: Thu, 19 Jan 2017 19:17:38 -0700 Subject: [PATCH 036/130] keep graph displayed when build is clicked so that multiple tabs can be opened from graph Also show spinner on every new request to let the user know that new data is inbound --- .../vue_pipelines_index/stage.js.es6 | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/vue_pipelines_index/stage.js.es6 b/app/assets/javascripts/vue_pipelines_index/stage.js.es6 index 32973132174..a9fd1c4b876 100644 --- a/app/assets/javascripts/vue_pipelines_index/stage.js.es6 +++ b/app/assets/javascripts/vue_pipelines_index/stage.js.es6 @@ -9,9 +9,24 @@ spinner: '', }; }, - props: ['stage', 'svgs', 'match'], + props: { + stage: { + type: Object, + required: true, + }, + svgs: { + type: DOMStringMap, + required: true, + }, + match: { + type: Function, + required: true, + }, + }, methods: { fetchBuilds(e) { + if (this.builds) this.builds = ''; + const areaExpanded = e.currentTarget.attributes['aria-expanded']; if (areaExpanded && (areaExpanded.textContent === 'true')) return null; @@ -24,6 +39,9 @@ return flash; }); }, + keepGraph(e) { + e.stopPropagation(); + }, }, computed: { buildsOrSpinner() { @@ -64,7 +82,7 @@