From 8e7f19c60bea4eec86844be1e0db12ebf30f105e Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 2 Dec 2017 22:55:49 -0800 Subject: [PATCH 01/50] Add button to run scheduled pipeline immediately Closes #38741 --- .../projects/pipeline_schedules_controller.rb | 16 ++++++++- app/helpers/gitlab_routing_helper.rb | 5 +++ .../_pipeline_schedule.html.haml | 4 ++- app/workers/run_pipeline_schedule_worker.rb | 20 +++++++++++ .../sh-add-schedule-pipeline-run-now.yml | 5 +++ config/routes/project.rb | 1 + .../pipeline_schedules_controller_spec.rb | 26 +++++++++++++- .../run_pipeline_schedule_worker_spec.rb | 36 +++++++++++++++++++ 8 files changed, 110 insertions(+), 3 deletions(-) create mode 100644 app/workers/run_pipeline_schedule_worker.rb create mode 100644 changelogs/unreleased/sh-add-schedule-pipeline-run-now.yml create mode 100644 spec/workers/run_pipeline_schedule_worker_spec.rb diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb index ec7c645df5a..9fc7935950d 100644 --- a/app/controllers/projects/pipeline_schedules_controller.rb +++ b/app/controllers/projects/pipeline_schedules_controller.rb @@ -1,9 +1,10 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController before_action :schedule, except: [:index, :new, :create] + before_action :authorize_create_pipeline!, only: [:run] before_action :authorize_read_pipeline_schedule! before_action :authorize_create_pipeline_schedule!, only: [:new, :create] - before_action :authorize_update_pipeline_schedule!, except: [:index, :new, :create] + before_action :authorize_update_pipeline_schedule!, except: [:index, :new, :create, :run] before_action :authorize_admin_pipeline_schedule!, only: [:destroy] def index @@ -40,6 +41,19 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController end end + def run + job_id = RunPipelineScheduleWorker.perform_async(schedule.id, current_user.id) + + flash[:notice] = + if job_id + 'Successfully scheduled pipeline to run immediately' + else + 'Unable to schedule a pipeline to run immediately' + end + + redirect_to pipeline_schedules_path(@project) + end + def take_ownership if schedule.update(owner: current_user) redirect_to pipeline_schedules_path(@project) diff --git a/app/helpers/gitlab_routing_helper.rb b/app/helpers/gitlab_routing_helper.rb index a77aa0ad2cc..5f72c6e64b6 100644 --- a/app/helpers/gitlab_routing_helper.rb +++ b/app/helpers/gitlab_routing_helper.rb @@ -182,6 +182,11 @@ module GitlabRoutingHelper edit_project_pipeline_schedule_path(project, schedule) end + def run_pipeline_schedule_path(schedule, *args) + project = schedule.project + run_project_pipeline_schedule_path(project, schedule, *args) + end + def take_ownership_pipeline_schedule_path(schedule, *args) project = schedule.project take_ownership_project_pipeline_schedule_path(project, schedule, *args) diff --git a/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml b/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml index bd8c38292d6..57f0ffba9ff 100644 --- a/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml +++ b/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml @@ -26,10 +26,12 @@ = pipeline_schedule.owner&.name %td .pull-right.btn-group + - if can?(current_user, :create_pipeline, @project) + = link_to run_pipeline_schedule_path(pipeline_schedule), method: :post, title: s_('Play'), class: 'btn' do + = icon('play') - if can?(current_user, :update_pipeline_schedule, pipeline_schedule) = link_to take_ownership_pipeline_schedule_path(pipeline_schedule), method: :post, title: s_('PipelineSchedules|Take ownership'), class: 'btn' do = s_('PipelineSchedules|Take ownership') - - if can?(current_user, :update_pipeline_schedule, pipeline_schedule) = link_to edit_pipeline_schedule_path(pipeline_schedule), title: _('Edit'), class: 'btn' do = icon('pencil') - if can?(current_user, :admin_pipeline_schedule, pipeline_schedule) diff --git a/app/workers/run_pipeline_schedule_worker.rb b/app/workers/run_pipeline_schedule_worker.rb new file mode 100644 index 00000000000..ac6371f0871 --- /dev/null +++ b/app/workers/run_pipeline_schedule_worker.rb @@ -0,0 +1,20 @@ +class RunPipelineScheduleWorker + include Sidekiq::Worker + include PipelineQueue + + enqueue_in group: :creation + + def perform(schedule_id, user_id) + schedule = Ci::PipelineSchedule.find(schedule_id) + user = User.find(user_id) + + run_pipeline_schedule(schedule, user) + end + + def run_pipeline_schedule(schedule, user) + Ci::CreatePipelineService.new(schedule.project, + user, + ref: schedule.ref) + .execute(:schedule, ignore_skip_ci: true, save_on_errors: false, schedule: schedule) + end +end diff --git a/changelogs/unreleased/sh-add-schedule-pipeline-run-now.yml b/changelogs/unreleased/sh-add-schedule-pipeline-run-now.yml new file mode 100644 index 00000000000..6d06f695f10 --- /dev/null +++ b/changelogs/unreleased/sh-add-schedule-pipeline-run-now.yml @@ -0,0 +1,5 @@ +--- +title: Add button to run scheduled pipeline immediately +merge_request: +author: +type: added diff --git a/config/routes/project.rb b/config/routes/project.rb index 093da10f57f..3dcb21e45c5 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -179,6 +179,7 @@ constraints(ProjectUrlConstrainer.new) do resources :pipeline_schedules, except: [:show] do member do + post :run post :take_ownership end end diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb index 4e52e261920..384a407a100 100644 --- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb +++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb @@ -96,7 +96,7 @@ describe Projects::PipelineSchedulesController do end end - context 'when variables_attributes has two variables and duplicted' do + context 'when variables_attributes has two variables and duplicated' do let(:schedule) do basic_param.merge({ variables_attributes: [{ key: 'AAA', value: 'AAA123' }, { key: 'AAA', value: 'BBB123' }] @@ -364,6 +364,30 @@ describe Projects::PipelineSchedulesController do end end + describe 'POST #run' do + set(:user) { create(:user) } + + context 'when a developer makes the request' do + before do + project.add_developer(user) + sign_in(user) + end + + it 'executes a new pipeline' do + expect(RunPipelineScheduleWorker).to receive(:perform_async).with(pipeline_schedule.id, user.id).and_return('job-123') + + go + + expect(flash[:notice]).to eq 'Successfully scheduled pipeline to run immediately' + expect(response).to have_gitlab_http_status(302) + end + end + + def go + post :run, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id + end + end + describe 'DELETE #destroy' do set(:user) { create(:user) } diff --git a/spec/workers/run_pipeline_schedule_worker_spec.rb b/spec/workers/run_pipeline_schedule_worker_spec.rb new file mode 100644 index 00000000000..4a88ac51f62 --- /dev/null +++ b/spec/workers/run_pipeline_schedule_worker_spec.rb @@ -0,0 +1,36 @@ +require 'spec_helper' + +describe RunPipelineScheduleWorker do + describe '#perform' do + let(:project) { create(:project) } + let(:worker) { described_class.new } + let(:user) { create(:user) } + let(:pipeline_schedule) { create(:ci_pipeline_schedule, :nightly, project: project ) } + + context 'when a project not found' do + it 'does not call the Service' do + expect(Ci::CreatePipelineService).not_to receive(:new) + expect { worker.perform(100000, user.id) }.to raise_error(ActiveRecord::RecordNotFound) + end + end + + context 'when a user not found' do + it 'does not call the Service' do + expect(Ci::CreatePipelineService).not_to receive(:new) + expect { worker.perform(pipeline_schedule.id, 10000) }.to raise_error(ActiveRecord::RecordNotFound) + end + end + + context 'when everything is ok' do + let(:project) { create(:project) } + let(:create_pipeline_service) { instance_double(Ci::CreatePipelineService) } + + it 'calls the Service' do + expect(Ci::CreatePipelineService).to receive(:new).with(project, user, ref: pipeline_schedule.ref).and_return(create_pipeline_service) + expect(create_pipeline_service).to receive(:execute).with(:schedule, ignore_skip_ci: true, save_on_errors: false, schedule: pipeline_schedule) + + worker.perform(pipeline_schedule.id, user.id) + end + end + end +end From f6966cfa63fab7e3c8847d69101c6c6a444fb85f Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 5 Dec 2017 22:24:20 -0800 Subject: [PATCH 02/50] Address some comments with running a pipeline schedule --- .../projects/pipeline_schedules_controller.rb | 6 +++--- app/helpers/gitlab_routing_helper.rb | 4 ++-- app/workers/run_pipeline_schedule_worker.rb | 6 ++++-- config/routes/project.rb | 2 +- .../pipeline_schedules_controller_spec.rb | 4 ++-- spec/workers/run_pipeline_schedule_worker_spec.rb | 15 +++++++++------ 6 files changed, 21 insertions(+), 16 deletions(-) diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb index 9fc7935950d..38edb38f9fc 100644 --- a/app/controllers/projects/pipeline_schedules_controller.rb +++ b/app/controllers/projects/pipeline_schedules_controller.rb @@ -1,10 +1,10 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController before_action :schedule, except: [:index, :new, :create] - before_action :authorize_create_pipeline!, only: [:run] + before_action :authorize_create_pipeline!, only: [:play] before_action :authorize_read_pipeline_schedule! before_action :authorize_create_pipeline_schedule!, only: [:new, :create] - before_action :authorize_update_pipeline_schedule!, except: [:index, :new, :create, :run] + before_action :authorize_update_pipeline_schedule!, except: [:index, :new, :create, :play] before_action :authorize_admin_pipeline_schedule!, only: [:destroy] def index @@ -41,7 +41,7 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController end end - def run + def play job_id = RunPipelineScheduleWorker.perform_async(schedule.id, current_user.id) flash[:notice] = diff --git a/app/helpers/gitlab_routing_helper.rb b/app/helpers/gitlab_routing_helper.rb index 5f72c6e64b6..7f3c118c7ab 100644 --- a/app/helpers/gitlab_routing_helper.rb +++ b/app/helpers/gitlab_routing_helper.rb @@ -182,9 +182,9 @@ module GitlabRoutingHelper edit_project_pipeline_schedule_path(project, schedule) end - def run_pipeline_schedule_path(schedule, *args) + def play_pipeline_schedule_path(schedule, *args) project = schedule.project - run_project_pipeline_schedule_path(project, schedule, *args) + play_project_pipeline_schedule_path(project, schedule, *args) end def take_ownership_pipeline_schedule_path(schedule, *args) diff --git a/app/workers/run_pipeline_schedule_worker.rb b/app/workers/run_pipeline_schedule_worker.rb index ac6371f0871..3e0d3fed20a 100644 --- a/app/workers/run_pipeline_schedule_worker.rb +++ b/app/workers/run_pipeline_schedule_worker.rb @@ -5,8 +5,10 @@ class RunPipelineScheduleWorker enqueue_in group: :creation def perform(schedule_id, user_id) - schedule = Ci::PipelineSchedule.find(schedule_id) - user = User.find(user_id) + schedule = Ci::PipelineSchedule.find_by(id: schedule_id) + user = User.find_by(id: user_id) + + return unless schedule && user run_pipeline_schedule(schedule, user) end diff --git a/config/routes/project.rb b/config/routes/project.rb index 3dcb21e45c5..239b5480321 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -179,7 +179,7 @@ constraints(ProjectUrlConstrainer.new) do resources :pipeline_schedules, except: [:show] do member do - post :run + post :play post :take_ownership end end diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb index 384a407a100..e875f5bce08 100644 --- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb +++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb @@ -364,7 +364,7 @@ describe Projects::PipelineSchedulesController do end end - describe 'POST #run' do + describe 'POST #play' do set(:user) { create(:user) } context 'when a developer makes the request' do @@ -384,7 +384,7 @@ describe Projects::PipelineSchedulesController do end def go - post :run, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id + post :play, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id end end diff --git a/spec/workers/run_pipeline_schedule_worker_spec.rb b/spec/workers/run_pipeline_schedule_worker_spec.rb index 4a88ac51f62..481a84837f9 100644 --- a/spec/workers/run_pipeline_schedule_worker_spec.rb +++ b/spec/workers/run_pipeline_schedule_worker_spec.rb @@ -2,27 +2,30 @@ require 'spec_helper' describe RunPipelineScheduleWorker do describe '#perform' do - let(:project) { create(:project) } + set(:project) { create(:project) } + set(:user) { create(:user) } + set(:pipeline_schedule) { create(:ci_pipeline_schedule, :nightly, project: project ) } let(:worker) { described_class.new } - let(:user) { create(:user) } - let(:pipeline_schedule) { create(:ci_pipeline_schedule, :nightly, project: project ) } context 'when a project not found' do it 'does not call the Service' do expect(Ci::CreatePipelineService).not_to receive(:new) - expect { worker.perform(100000, user.id) }.to raise_error(ActiveRecord::RecordNotFound) + expect(worker).not_to receive(:run_pipeline_schedule) + + worker.perform(100000, user.id) end end context 'when a user not found' do it 'does not call the Service' do expect(Ci::CreatePipelineService).not_to receive(:new) - expect { worker.perform(pipeline_schedule.id, 10000) }.to raise_error(ActiveRecord::RecordNotFound) + expect(worker).not_to receive(:run_pipeline_schedule) + + worker.perform(pipeline_schedule.id, 10000) end end context 'when everything is ok' do - let(:project) { create(:project) } let(:create_pipeline_service) { instance_double(Ci::CreatePipelineService) } it 'calls the Service' do From bc2d32aca0be46250bd02c9312d1064df024b621 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 5 Dec 2017 23:23:59 -0800 Subject: [PATCH 03/50] Create a play_pipeline_schedule policy and use it --- .../projects/pipeline_schedules_controller.rb | 6 +++++- app/policies/ci/pipeline_schedule_policy.rb | 18 ++++++++++++++++ .../_pipeline_schedule.html.haml | 2 +- .../pipeline_schedules_controller_spec.rb | 21 ++++++++++++++----- 4 files changed, 40 insertions(+), 7 deletions(-) diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb index 38edb38f9fc..a4e865cb9da 100644 --- a/app/controllers/projects/pipeline_schedules_controller.rb +++ b/app/controllers/projects/pipeline_schedules_controller.rb @@ -1,7 +1,7 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController before_action :schedule, except: [:index, :new, :create] - before_action :authorize_create_pipeline!, only: [:play] + before_action :authorize_play_pipeline_schedule!, only: [:play] before_action :authorize_read_pipeline_schedule! before_action :authorize_create_pipeline_schedule!, only: [:new, :create] before_action :authorize_update_pipeline_schedule!, except: [:index, :new, :create, :play] @@ -84,6 +84,10 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController variables_attributes: [:id, :key, :value, :_destroy] ) end + def authorize_play_pipeline_schedule! + return access_denied! unless can?(current_user, :play_pipeline_schedule, schedule) + end + def authorize_update_pipeline_schedule! return access_denied! unless can?(current_user, :update_pipeline_schedule, schedule) end diff --git a/app/policies/ci/pipeline_schedule_policy.rb b/app/policies/ci/pipeline_schedule_policy.rb index 6b7598e1821..8e7e129f135 100644 --- a/app/policies/ci/pipeline_schedule_policy.rb +++ b/app/policies/ci/pipeline_schedule_policy.rb @@ -2,13 +2,31 @@ module Ci class PipelineSchedulePolicy < PipelinePolicy alias_method :pipeline_schedule, :subject + condition(:protected_ref) do + access = ::Gitlab::UserAccess.new(@user, project: @subject.project) + + if @subject.project.repository.branch_exists?(@subject.ref) + access.can_update_branch?(@subject.ref) + elsif @subject.project.repository.tag_exists?(@subject.ref) + access.can_create_tag?(@subject.ref) + else + true + end + end + condition(:owner_of_schedule) do can?(:developer_access) && pipeline_schedule.owned_by?(@user) end + rule { can?(:developer_access) }.policy do + enable :play_pipeline_schedule + end + rule { can?(:master_access) | owner_of_schedule }.policy do enable :update_pipeline_schedule enable :admin_pipeline_schedule end + + rule { protected_ref }.prevent :play_pipeline_schedule end end diff --git a/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml b/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml index 57f0ffba9ff..12d7e81c39e 100644 --- a/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml +++ b/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml @@ -26,7 +26,7 @@ = pipeline_schedule.owner&.name %td .pull-right.btn-group - - if can?(current_user, :create_pipeline, @project) + - if can?(current_user, :play_pipeline_schedule, pipeline_schedule) = link_to run_pipeline_schedule_path(pipeline_schedule), method: :post, title: s_('Play'), class: 'btn' do = icon('play') - if can?(current_user, :update_pipeline_schedule, pipeline_schedule) diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb index e875f5bce08..3878b0a5e4f 100644 --- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb +++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb @@ -3,8 +3,8 @@ require 'spec_helper' describe Projects::PipelineSchedulesController do include AccessMatchersForController - set(:project) { create(:project, :public) } - let!(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project) } + set(:project) { create(:project, :public, :repository) } + set(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project) } describe 'GET #index' do let(:scope) { nil } @@ -366,25 +366,36 @@ describe Projects::PipelineSchedulesController do describe 'POST #play' do set(:user) { create(:user) } + let(:ref) { 'master' } context 'when a developer makes the request' do before do project.add_developer(user) + sign_in(user) end it 'executes a new pipeline' do expect(RunPipelineScheduleWorker).to receive(:perform_async).with(pipeline_schedule.id, user.id).and_return('job-123') - go + post :play, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id expect(flash[:notice]).to eq 'Successfully scheduled pipeline to run immediately' expect(response).to have_gitlab_http_status(302) end end - def go - post :play, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id + context 'when a developer attempts to schedule a protected ref' do + it 'does not allow pipeline to be executed' do + create(:protected_branch, project: project, name: ref) + protected_schedule = create(:ci_pipeline_schedule, project: project, ref: ref) + + expect(RunPipelineScheduleWorker).not_to receive(:perform_async) + + post :play, namespace_id: project.namespace.to_param, project_id: project, id: protected_schedule.id + + expect(response).to have_gitlab_http_status(404) + end end end From 87118872c94ab465d400090e247b1e4ae90c2d82 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 5 Dec 2017 23:36:49 -0800 Subject: [PATCH 04/50] Fix conditions for checking pipeline schedule rules --- app/policies/ci/pipeline_schedule_policy.rb | 6 +++--- .../pipeline_schedules/_pipeline_schedule.html.haml | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/policies/ci/pipeline_schedule_policy.rb b/app/policies/ci/pipeline_schedule_policy.rb index 8e7e129f135..c0a2bb963e9 100644 --- a/app/policies/ci/pipeline_schedule_policy.rb +++ b/app/policies/ci/pipeline_schedule_policy.rb @@ -6,11 +6,11 @@ module Ci access = ::Gitlab::UserAccess.new(@user, project: @subject.project) if @subject.project.repository.branch_exists?(@subject.ref) - access.can_update_branch?(@subject.ref) + !access.can_update_branch?(@subject.ref) elsif @subject.project.repository.tag_exists?(@subject.ref) - access.can_create_tag?(@subject.ref) + !access.can_create_tag?(@subject.ref) else - true + false end end diff --git a/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml b/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml index 12d7e81c39e..f8c4005a9e0 100644 --- a/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml +++ b/app/views/projects/pipeline_schedules/_pipeline_schedule.html.haml @@ -27,7 +27,7 @@ %td .pull-right.btn-group - if can?(current_user, :play_pipeline_schedule, pipeline_schedule) - = link_to run_pipeline_schedule_path(pipeline_schedule), method: :post, title: s_('Play'), class: 'btn' do + = link_to play_pipeline_schedule_path(pipeline_schedule), method: :post, title: s_('Play'), class: 'btn' do = icon('play') - if can?(current_user, :update_pipeline_schedule, pipeline_schedule) = link_to take_ownership_pipeline_schedule_path(pipeline_schedule), method: :post, title: s_('PipelineSchedules|Take ownership'), class: 'btn' do From ad3732955389024b8a209455f9a889d5ebf411b9 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Thu, 7 Dec 2017 23:49:55 -0800 Subject: [PATCH 05/50] Refactor common protected ref check --- app/policies/ci/pipeline_policy.rb | 20 ++-- app/policies/ci/pipeline_schedule_policy.rb | 10 +- .../ci/pipeline_schedule_policy_spec.rb | 92 +++++++++++++++++++ 3 files changed, 104 insertions(+), 18 deletions(-) create mode 100644 spec/policies/ci/pipeline_schedule_policy_spec.rb diff --git a/app/policies/ci/pipeline_policy.rb b/app/policies/ci/pipeline_policy.rb index 4e689a9efd5..6363c382ff8 100644 --- a/app/policies/ci/pipeline_policy.rb +++ b/app/policies/ci/pipeline_policy.rb @@ -2,16 +2,18 @@ module Ci class PipelinePolicy < BasePolicy delegate { @subject.project } - condition(:protected_ref) do - access = ::Gitlab::UserAccess.new(@user, project: @subject.project) - - if @subject.tag? - !access.can_create_tag?(@subject.ref) - else - !access.can_update_branch?(@subject.ref) - end - end + condition(:protected_ref) { ref_protected?(@user, @subject.project, @subject.tag?, @subject.ref) } rule { protected_ref }.prevent :update_pipeline + + def ref_protected?(user, project, tag, ref) + access = ::Gitlab::UserAccess.new(user, project: project) + + if tag + !access.can_create_tag?(ref) + else + !access.can_update_branch?(ref) + end + end end end diff --git a/app/policies/ci/pipeline_schedule_policy.rb b/app/policies/ci/pipeline_schedule_policy.rb index c0a2bb963e9..abcf536b2f7 100644 --- a/app/policies/ci/pipeline_schedule_policy.rb +++ b/app/policies/ci/pipeline_schedule_policy.rb @@ -3,15 +3,7 @@ module Ci alias_method :pipeline_schedule, :subject condition(:protected_ref) do - access = ::Gitlab::UserAccess.new(@user, project: @subject.project) - - if @subject.project.repository.branch_exists?(@subject.ref) - !access.can_update_branch?(@subject.ref) - elsif @subject.project.repository.tag_exists?(@subject.ref) - !access.can_create_tag?(@subject.ref) - else - false - end + ref_protected?(@user, @subject.project, @subject.project.repository.tag_exists?(@subject.ref), @subject.ref) end condition(:owner_of_schedule) do diff --git a/spec/policies/ci/pipeline_schedule_policy_spec.rb b/spec/policies/ci/pipeline_schedule_policy_spec.rb new file mode 100644 index 00000000000..1b0e9fac355 --- /dev/null +++ b/spec/policies/ci/pipeline_schedule_policy_spec.rb @@ -0,0 +1,92 @@ +require 'spec_helper' + +describe Ci::PipelineSchedulePolicy, :models do + set(:user) { create(:user) } + set(:project) { create(:project, :repository) } + set(:pipeline_schedule) { create(:ci_pipeline_schedule, :nightly, project: project) } + + let(:policy) do + described_class.new(user, pipeline_schedule) + end + + describe 'rules' do + describe 'rules for protected ref' do + before do + project.add_developer(user) + end + + context 'when no one can push or merge to the branch' do + before do + create(:protected_branch, :no_one_can_push, + name: pipeline_schedule.ref, project: project) + end + + it 'does not include ability to play pipeline schedule' do + expect(policy).to be_disallowed :play_pipeline_schedule + end + end + + context 'when developers can push to the branch' do + before do + create(:protected_branch, :developers_can_merge, + name: pipeline_schedule.ref, project: project) + end + + it 'includes ability to update pipeline' do + expect(policy).to be_allowed :play_pipeline_schedule + end + end + + context 'when no one can create the tag' do + let(:tag) { 'v1.0.0' } + + before do + pipeline_schedule.update(ref: tag) + + create(:protected_tag, :no_one_can_create, + name: pipeline_schedule.ref, project: project) + end + + it 'does not include ability to play pipeline schedule' do + expect(policy).to be_disallowed :play_pipeline_schedule + end + end + + context 'when no one can create the tag but it is not a tag' do + before do + create(:protected_tag, :no_one_can_create, + name: pipeline_schedule.ref, project: project) + end + + it 'includes ability to play pipeline schedule' do + expect(policy).to be_allowed :play_pipeline_schedule + end + end + end + + describe 'rules for owner of schedule' do + before do + project.add_developer(user) + pipeline_schedule.update(owner: user) + end + + it 'includes abilities to do do all operations on pipeline schedule' do + expect(policy).to be_allowed :play_pipeline_schedule + expect(policy).to be_allowed :update_pipeline_schedule + expect(policy).to be_allowed :admin_pipeline_schedule + end + end + + describe 'rules for a master' do + before do + project.add_master(user) + end + + it 'includes abilities to do do all operations on pipeline schedule' do + expect(policy).to be_allowed :play_pipeline_schedule + expect(policy).to be_allowed :update_pipeline_schedule + expect(policy).to be_allowed :admin_pipeline_schedule + end + end + end +end From 0ea70802e19cbe11c6af0f6750200bb137225940 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Thu, 7 Dec 2017 23:58:05 -0800 Subject: [PATCH 06/50] Fix Sidekiq worker and make flash message return a link to the pipelines page --- app/controllers/projects/pipeline_schedules_controller.rb | 2 +- app/workers/run_pipeline_schedule_worker.rb | 2 +- spec/controllers/projects/pipeline_schedules_controller_spec.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb index a4e865cb9da..fe77d8eabeb 100644 --- a/app/controllers/projects/pipeline_schedules_controller.rb +++ b/app/controllers/projects/pipeline_schedules_controller.rb @@ -46,7 +46,7 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController flash[:notice] = if job_id - 'Successfully scheduled pipeline to run immediately' + "Successfully scheduled a pipeline to run. Go to the Pipelines page for details".html_safe else 'Unable to schedule a pipeline to run immediately' end diff --git a/app/workers/run_pipeline_schedule_worker.rb b/app/workers/run_pipeline_schedule_worker.rb index 3e0d3fed20a..7725ad319a3 100644 --- a/app/workers/run_pipeline_schedule_worker.rb +++ b/app/workers/run_pipeline_schedule_worker.rb @@ -1,5 +1,5 @@ class RunPipelineScheduleWorker - include Sidekiq::Worker + include ApplicationWorker include PipelineQueue enqueue_in group: :creation diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb index 3878b0a5e4f..debc20ee467 100644 --- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb +++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb @@ -380,7 +380,7 @@ describe Projects::PipelineSchedulesController do post :play, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id - expect(flash[:notice]).to eq 'Successfully scheduled pipeline to run immediately' + expect(flash[:notice]).to start_with 'Successfully scheduled a pipeline to run' expect(response).to have_gitlab_http_status(302) end end From f8c3a58a54d622193a0cf15777a0d0631289278c Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 8 Dec 2017 22:20:28 -0800 Subject: [PATCH 07/50] Avoid Gitaly N+1 calls by caching tag_names --- app/models/repository.rb | 6 ++++++ .../projects/pipeline_schedules_controller_spec.rb | 2 ++ spec/models/repository_spec.rb | 9 +++++++++ 3 files changed, 17 insertions(+) diff --git a/app/models/repository.rb b/app/models/repository.rb index c0e31eca8da..2413e60bc76 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -221,6 +221,12 @@ class Repository branch_names.include?(branch_name) end + def tag_exists?(tag_name) + return false unless raw_repository + + tag_names.include?(tag_name) + end + def ref_exists?(ref) !!raw_repository&.ref_exists?(ref) rescue ArgumentError diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb index debc20ee467..8888573e882 100644 --- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb +++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb @@ -7,6 +7,8 @@ describe Projects::PipelineSchedulesController do set(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project) } describe 'GET #index' do + render_views + let(:scope) { nil } let!(:inactive_pipeline_schedule) do create(:ci_pipeline_schedule, :inactive, project: project) diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 129fce74f45..08eb563082f 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1157,6 +1157,15 @@ describe Repository do end end + describe '#tag_exists?' do + it 'uses tag_names' do + allow(repository).to receive(:tag_names).and_return(['foobar']) + + expect(repository.tag_exists?('foobar')).to eq(true) + expect(repository.tag_exists?('master')).to eq(false) + end + end + describe '#branch_names', :use_clean_rails_memory_store_caching do let(:fake_branch_names) { ['foobar'] } From 54f13b1ec8542dc5085e0367734e8344c2c3d01e Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 9 Dec 2017 01:01:42 -0800 Subject: [PATCH 08/50] Add rate limiting to guard against excessive scheduling of pipelines --- .../projects/pipeline_schedules_controller.rb | 11 +++++++ lib/gitlab/action_rate_limiter.rb | 31 +++++++++++++++++++ .../pipeline_schedules_controller_spec.rb | 2 +- 3 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 lib/gitlab/action_rate_limiter.rb diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb index fe77d8eabeb..b7a0a3591cd 100644 --- a/app/controllers/projects/pipeline_schedules_controller.rb +++ b/app/controllers/projects/pipeline_schedules_controller.rb @@ -42,6 +42,13 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController end def play + limiter = ::Gitlab::ActionRateLimiter.new(action: 'play_pipeline_schedule') + + if limiter.throttled?(throttle_key, 1) + flash[:notice] = 'You cannot play this scheduled pipeline at the moment. Please wait a minute.' + return redirect_to pipeline_schedules_path(@project) + end + job_id = RunPipelineScheduleWorker.perform_async(schedule.id, current_user.id) flash[:notice] = @@ -74,6 +81,10 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController private + def throttle_key + "user:#{current_user.id}:schedule:#{schedule.id}" + end + def schedule @schedule ||= project.pipeline_schedules.find(params[:id]) end diff --git a/lib/gitlab/action_rate_limiter.rb b/lib/gitlab/action_rate_limiter.rb new file mode 100644 index 00000000000..c3af583a3ed --- /dev/null +++ b/lib/gitlab/action_rate_limiter.rb @@ -0,0 +1,31 @@ +module Gitlab + # This class implements a simple rate limiter that can be used to throttle + # certain actions. Unlike Rack Attack and Rack::Throttle, which operate at + # the middleware level, this can be used at the controller level. + class ActionRateLimiter + TIME_TO_EXPIRE = 60 # 1 min + + attr_accessor :action, :expiry_time + + def initialize(action:, expiry_time: TIME_TO_EXPIRE) + @action = action + @expiry_time = expiry_time + end + + def increment(key) + value = 0 + + Gitlab::Redis::Cache.with do |redis| + cache_key = "action_rate_limiter:#{action}:#{key}" + value = redis.incr(cache_key) + redis.expire(cache_key, expiry_time) if value == 1 + end + + value.to_i + end + + def throttled?(key, threshold_value) + self.increment(key) > threshold_value + end + end +end diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb index 8888573e882..844c62ef005 100644 --- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb +++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb @@ -366,7 +366,7 @@ describe Projects::PipelineSchedulesController do end end - describe 'POST #play' do + describe 'POST #play', :clean_gitlab_redis_cache do set(:user) { create(:user) } let(:ref) { 'master' } From ef78f67f4a2e19d204f5f4d4770649be1fe7bee9 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 9 Dec 2017 15:08:27 -0800 Subject: [PATCH 09/50] Add a spec for rate limiting pipeline schedules --- .../projects/pipeline_schedules_controller.rb | 4 ++-- .../projects/pipeline_schedules_controller_spec.rb | 10 ++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb index b7a0a3591cd..87878667e9b 100644 --- a/app/controllers/projects/pipeline_schedules_controller.rb +++ b/app/controllers/projects/pipeline_schedules_controller.rb @@ -45,7 +45,7 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController limiter = ::Gitlab::ActionRateLimiter.new(action: 'play_pipeline_schedule') if limiter.throttled?(throttle_key, 1) - flash[:notice] = 'You cannot play this scheduled pipeline at the moment. Please wait a minute.' + flash[:alert] = 'You cannot play this scheduled pipeline at the moment. Please wait a minute.' return redirect_to pipeline_schedules_path(@project) end @@ -53,7 +53,7 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController flash[:notice] = if job_id - "Successfully scheduled a pipeline to run. Go to the Pipelines page for details".html_safe + "Successfully scheduled a pipeline to run. Go to the Pipelines page for details.".html_safe else 'Unable to schedule a pipeline to run immediately' end diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb index 844c62ef005..ffc1259eb8f 100644 --- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb +++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb @@ -385,6 +385,16 @@ describe Projects::PipelineSchedulesController do expect(flash[:notice]).to start_with 'Successfully scheduled a pipeline to run' expect(response).to have_gitlab_http_status(302) end + + it 'prevents users from scheduling the same pipeline repeatedly' do + 2.times do + post :play, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id + end + + expect(flash.to_a.size).to eq(2) + expect(flash[:alert]).to eq 'You cannot play this scheduled pipeline at the moment. Please wait a minute.' + expect(response).to have_gitlab_http_status(302) + end end context 'when a developer attempts to schedule a protected ref' do From 8b939bfab769810ba56f5f4ca18632fd7ae435db Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 9 Dec 2017 15:45:01 -0800 Subject: [PATCH 10/50] Add spec for ActionRateLimiter --- lib/gitlab/action_rate_limiter.rb | 4 +-- spec/lib/gitlab/action_rate_limiter_spec.rb | 27 +++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) create mode 100644 spec/lib/gitlab/action_rate_limiter_spec.rb diff --git a/lib/gitlab/action_rate_limiter.rb b/lib/gitlab/action_rate_limiter.rb index c3af583a3ed..7b231ac14ca 100644 --- a/lib/gitlab/action_rate_limiter.rb +++ b/lib/gitlab/action_rate_limiter.rb @@ -16,12 +16,12 @@ module Gitlab value = 0 Gitlab::Redis::Cache.with do |redis| - cache_key = "action_rate_limiter:#{action}:#{key}" + cache_key = "action_rate_limiter:#{action.to_s}:#{key}" value = redis.incr(cache_key) redis.expire(cache_key, expiry_time) if value == 1 end - value.to_i + value end def throttled?(key, threshold_value) diff --git a/spec/lib/gitlab/action_rate_limiter_spec.rb b/spec/lib/gitlab/action_rate_limiter_spec.rb new file mode 100644 index 00000000000..84661605623 --- /dev/null +++ b/spec/lib/gitlab/action_rate_limiter_spec.rb @@ -0,0 +1,27 @@ +require 'spec_helper' + +describe Gitlab::ActionRateLimiter do + let(:redis) { double('redis') } + let(:key) { 'user:1' } + let(:cache_key) { "action_rate_limiter:test_action:#{key}" } + + subject { described_class.new(action: :test_action, expiry_time: 100) } + + before do + allow(Gitlab::Redis::Cache).to receive(:with).and_yield(redis) + end + + it 'increases the throttle count and sets the expire time' do + expect(redis).to receive(:incr).with(cache_key).and_return(1) + expect(redis).to receive(:expire).with(cache_key, 100) + + expect(subject.throttled?(key, 1)).to be false + end + + it 'returns true if the key is throttled' do + expect(redis).to receive(:incr).with(cache_key).and_return(2) + expect(redis).not_to receive(:expire) + + expect(subject.throttled?(key, 1)).to be true + end +end From 02e7e499d4011733c1cf0f6fb675dd2d309f0d52 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 9 Dec 2017 21:00:34 -0800 Subject: [PATCH 11/50] Fix Rubocop offense and use a symbol instead of a string --- app/controllers/projects/pipeline_schedules_controller.rb | 2 +- lib/gitlab/action_rate_limiter.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb index 87878667e9b..dd6593650e7 100644 --- a/app/controllers/projects/pipeline_schedules_controller.rb +++ b/app/controllers/projects/pipeline_schedules_controller.rb @@ -42,7 +42,7 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController end def play - limiter = ::Gitlab::ActionRateLimiter.new(action: 'play_pipeline_schedule') + limiter = ::Gitlab::ActionRateLimiter.new(action: :play_pipeline_schedule) if limiter.throttled?(throttle_key, 1) flash[:alert] = 'You cannot play this scheduled pipeline at the moment. Please wait a minute.' diff --git a/lib/gitlab/action_rate_limiter.rb b/lib/gitlab/action_rate_limiter.rb index 7b231ac14ca..53add503c60 100644 --- a/lib/gitlab/action_rate_limiter.rb +++ b/lib/gitlab/action_rate_limiter.rb @@ -16,7 +16,7 @@ module Gitlab value = 0 Gitlab::Redis::Cache.with do |redis| - cache_key = "action_rate_limiter:#{action.to_s}:#{key}" + cache_key = "action_rate_limiter:#{action}:#{key}" value = redis.incr(cache_key) redis.expire(cache_key, expiry_time) if value == 1 end From 4b0465f20de1bf58326c7daf6876b63438f00d84 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 12 Dec 2017 15:46:05 -0800 Subject: [PATCH 12/50] Address review comments with playing pipeline scheduler --- .../projects/pipeline_schedules_controller.rb | 30 +++++++++---------- lib/gitlab/action_rate_limiter.rb | 18 ++++++++++- .../pipeline_schedules_controller_spec.rb | 22 +++++++++++--- spec/lib/gitlab/action_rate_limiter_spec.rb | 6 ++-- 4 files changed, 54 insertions(+), 22 deletions(-) diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb index dd6593650e7..b478e7b5e05 100644 --- a/app/controllers/projects/pipeline_schedules_controller.rb +++ b/app/controllers/projects/pipeline_schedules_controller.rb @@ -1,6 +1,7 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController before_action :schedule, except: [:index, :new, :create] + before_action :play_rate_limit, only: [:play] before_action :authorize_play_pipeline_schedule!, only: [:play] before_action :authorize_read_pipeline_schedule! before_action :authorize_create_pipeline_schedule!, only: [:new, :create] @@ -42,21 +43,13 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController end def play - limiter = ::Gitlab::ActionRateLimiter.new(action: :play_pipeline_schedule) - - if limiter.throttled?(throttle_key, 1) - flash[:alert] = 'You cannot play this scheduled pipeline at the moment. Please wait a minute.' - return redirect_to pipeline_schedules_path(@project) - end - job_id = RunPipelineScheduleWorker.perform_async(schedule.id, current_user.id) - flash[:notice] = - if job_id - "Successfully scheduled a pipeline to run. Go to the Pipelines page for details.".html_safe - else - 'Unable to schedule a pipeline to run immediately' - end + if job_id + flash[:notice] = "Successfully scheduled a pipeline to run. Go to the Pipelines page for details.".html_safe + else + flash[:alert] = 'Unable to schedule a pipeline to run immediately' + end redirect_to pipeline_schedules_path(@project) end @@ -81,8 +74,15 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController private - def throttle_key - "user:#{current_user.id}:schedule:#{schedule.id}" + def play_rate_limit + return unless current_user + + limiter = ::Gitlab::ActionRateLimiter.new(action: :play_pipeline_schedule) + + return unless limiter.throttled?([current_user, schedule], 1) + + flash[:alert] = 'You cannot play this scheduled pipeline at the moment. Please wait a minute.' + redirect_to pipeline_schedules_path(@project) end def schedule diff --git a/lib/gitlab/action_rate_limiter.rb b/lib/gitlab/action_rate_limiter.rb index 53add503c60..4cd3bdefda3 100644 --- a/lib/gitlab/action_rate_limiter.rb +++ b/lib/gitlab/action_rate_limiter.rb @@ -12,11 +12,15 @@ module Gitlab @expiry_time = expiry_time end + # Increments the given cache key and increments the value by 1 with the + # given expiration time. Returns the incremented value. + # + # key - An array of ActiveRecord instances def increment(key) value = 0 Gitlab::Redis::Cache.with do |redis| - cache_key = "action_rate_limiter:#{action}:#{key}" + cache_key = action_key(key) value = redis.incr(cache_key) redis.expire(cache_key, expiry_time) if value == 1 end @@ -24,8 +28,20 @@ module Gitlab value end + # Increments the given key and returns true if the action should + # be throttled. + # + # key - An array of ActiveRecord instances + # threshold_value - The maximum number of times this action should occur in the given time interval def throttled?(key, threshold_value) self.increment(key) > threshold_value end + + private + + def action_key(key) + serialized = key.map { |obj| "#{obj.class.model_name.to_s.underscore}:#{obj.id}" }.join(":") + "action_rate_limiter:#{action}:#{serialized}" + end end end diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb index ffc1259eb8f..966ffdf6996 100644 --- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb +++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb @@ -370,13 +370,27 @@ describe Projects::PipelineSchedulesController do set(:user) { create(:user) } let(:ref) { 'master' } - context 'when a developer makes the request' do - before do - project.add_developer(user) + before do + project.add_developer(user) - sign_in(user) + sign_in(user) + end + + context 'when an anonymous user makes the request' do + before do + sign_out(user) end + it 'does not allow pipeline to be executed' do + expect(RunPipelineScheduleWorker).not_to receive(:perform_async) + + post :play, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id + + expect(response).to have_gitlab_http_status(404) + end + end + + context 'when a developer makes the request' do it 'executes a new pipeline' do expect(RunPipelineScheduleWorker).to receive(:perform_async).with(pipeline_schedule.id, user.id).and_return('job-123') diff --git a/spec/lib/gitlab/action_rate_limiter_spec.rb b/spec/lib/gitlab/action_rate_limiter_spec.rb index 84661605623..542fc03e555 100644 --- a/spec/lib/gitlab/action_rate_limiter_spec.rb +++ b/spec/lib/gitlab/action_rate_limiter_spec.rb @@ -2,8 +2,10 @@ require 'spec_helper' describe Gitlab::ActionRateLimiter do let(:redis) { double('redis') } - let(:key) { 'user:1' } - let(:cache_key) { "action_rate_limiter:test_action:#{key}" } + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:key) { [user, project] } + let(:cache_key) { "action_rate_limiter:test_action:user:#{user.id}:project:#{project.id}" } subject { described_class.new(action: :test_action, expiry_time: 100) } From eaf2f48dc7db706fa1dea050543667f8cdebd4c4 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 15 Dec 2017 10:21:49 +0000 Subject: [PATCH 13/50] Export and use Notes ES module --- .../components/diff_note_avatars.js | 4 ++-- app/assets/javascripts/init_notes.js | 6 ++++-- app/assets/javascripts/main.js | 2 -- app/assets/javascripts/merge_request_tabs.js | 4 ++-- app/assets/javascripts/notes.js | 6 ++++++ spec/javascripts/merge_request_notes_spec.js | 4 +--- spec/javascripts/merge_request_tabs_spec.js | 19 +++++++++---------- spec/javascripts/notes_spec.js | 4 +--- 8 files changed, 25 insertions(+), 24 deletions(-) diff --git a/app/assets/javascripts/diff_notes/components/diff_note_avatars.js b/app/assets/javascripts/diff_notes/components/diff_note_avatars.js index 06ce84d7599..300b02da663 100644 --- a/app/assets/javascripts/diff_notes/components/diff_note_avatars.js +++ b/app/assets/javascripts/diff_notes/components/diff_note_avatars.js @@ -1,8 +1,8 @@ /* global CommentsStore */ -/* global notes */ import Vue from 'vue'; import collapseIcon from '../icons/collapse_icon.svg'; +import Notes from '../../notes'; import userAvatarImage from '../../vue_shared/components/user_avatar/user_avatar_image.vue'; const DiffNoteAvatars = Vue.extend({ @@ -129,7 +129,7 @@ const DiffNoteAvatars = Vue.extend({ }, methods: { clickedAvatar(e) { - notes.onAddDiffNote(e); + Notes.instance.onAddDiffNote(e); // Toggle the active state of the toggle all button this.toggleDiscussionsToggleState(); diff --git a/app/assets/javascripts/init_notes.js b/app/assets/javascripts/init_notes.js index 3a8b4360cb6..882aedfcc76 100644 --- a/app/assets/javascripts/init_notes.js +++ b/app/assets/javascripts/init_notes.js @@ -1,4 +1,4 @@ -/* global Notes */ +import Notes from './notes'; export default () => { const dataEl = document.querySelector('.js-notes-data'); @@ -10,5 +10,7 @@ export default () => { autocomplete, } = JSON.parse(dataEl.innerHTML); - window.notes = new Notes(notesUrl, notesIds, now, diffView, autocomplete); + // Create a singleton so that we don't need to assign + // into the window object, we can just access the current isntance with Notes.instance + Notes.initialize(notesUrl, notesIds, now, diffView, autocomplete); }; diff --git a/app/assets/javascripts/main.js b/app/assets/javascripts/main.js index ae3f76873cf..63f2a65700c 100644 --- a/app/assets/javascripts/main.js +++ b/app/assets/javascripts/main.js @@ -46,9 +46,7 @@ import LazyLoader from './lazy_loader'; import './line_highlighter'; import initLogoAnimation from './logo'; import './merge_request'; -import './merge_request_tabs'; import './milestone_select'; -import './notes'; import './preview_markdown'; import './projects_dropdown'; import './render_gfm'; diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index cacca35ca98..acfc62fe5cb 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -1,5 +1,4 @@ /* eslint-disable no-new, class-methods-use-this */ -/* global notes */ import Cookies from 'js-cookie'; import Flash from './flash'; @@ -16,6 +15,7 @@ import initDiscussionTab from './image_diff/init_discussion_tab'; import Diff from './diff'; import { localTimeAgo } from './lib/utils/datetime_utility'; import syntaxHighlight from './syntax_highlight'; +import Notes from './notes'; /* eslint-disable max-len */ // MergeRequestTabs @@ -324,7 +324,7 @@ export default class MergeRequestTabs { if (anchor && anchor.length > 0) { const notesContent = anchor.closest('.notes_content'); const lineType = notesContent.hasClass('new') ? 'new' : 'old'; - notes.toggleDiffNote({ + Notes.instance.toggleDiffNote({ target: anchor, lineType, forceShow: true, diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 042fe44e1c6..a2b8e6f6495 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -37,6 +37,12 @@ const MAX_VISIBLE_COMMIT_LIST_COUNT = 3; const REGEX_QUICK_ACTIONS = /^\/\w+.*$/gm; export default class Notes { + static initialize(notes_url, note_ids, last_fetched_at, view, enableGFM = true) { + if (!this.instance) { + this.instance = new Notes(notes_url, note_ids, last_fetched_at, view, enableGFM); + } + } + constructor(notes_url, note_ids, last_fetched_at, view, enableGFM = true) { this.updateTargetButtons = this.updateTargetButtons.bind(this); this.updateComment = this.updateComment.bind(this); diff --git a/spec/javascripts/merge_request_notes_spec.js b/spec/javascripts/merge_request_notes_spec.js index 6054b75d0b8..e983e4de3fc 100644 --- a/spec/javascripts/merge_request_notes_spec.js +++ b/spec/javascripts/merge_request_notes_spec.js @@ -1,11 +1,9 @@ -/* global Notes */ - import 'autosize'; import '~/gl_form'; import '~/lib/utils/text_utility'; import '~/render_gfm'; import '~/render_math'; -import '~/notes'; +import Notes from '~/notes'; const upArrowKeyCode = 38; diff --git a/spec/javascripts/merge_request_tabs_spec.js b/spec/javascripts/merge_request_tabs_spec.js index 31426ceb110..050f0ea9ebd 100644 --- a/spec/javascripts/merge_request_tabs_spec.js +++ b/spec/javascripts/merge_request_tabs_spec.js @@ -1,5 +1,4 @@ /* eslint-disable no-var, comma-dangle, object-shorthand */ -/* global Notes */ import * as urlUtils from '~/lib/utils/url_utility'; import MergeRequestTabs from '~/merge_request_tabs'; @@ -7,7 +6,7 @@ import '~/commit/pipelines/pipelines_bundle'; import '~/breakpoints'; import '~/lib/utils/common_utils'; import Diff from '~/diff'; -import '~/notes'; +import Notes from '~/notes'; import 'vendor/jquery.scrollTo'; (function () { @@ -279,8 +278,8 @@ import 'vendor/jquery.scrollTo'; loadFixtures('merge_requests/diff_comment.html.raw'); $('body').attr('data-page', 'projects:merge_requests:show'); window.gl.ImageFile = () => {}; - window.notes = new Notes('', []); - spyOn(window.notes, 'toggleDiffNote').and.callThrough(); + Notes.initialize('', []); + spyOn(Notes.instance, 'toggleDiffNote').and.callThrough(); }); afterEach(() => { @@ -338,7 +337,7 @@ import 'vendor/jquery.scrollTo'; this.class.loadDiff('/foo/bar/merge_requests/1/diffs'); expect(noteId.length).toBeGreaterThan(0); - expect(window.notes.toggleDiffNote).toHaveBeenCalledWith({ + expect(Notes.instance.toggleDiffNote).toHaveBeenCalledWith({ target: jasmine.any(Object), lineType: 'old', forceShow: true, @@ -349,7 +348,7 @@ import 'vendor/jquery.scrollTo'; spyOn(urlUtils, 'getLocationHash').and.returnValue('note_something-that-does-not-exist'); this.class.loadDiff('/foo/bar/merge_requests/1/diffs'); - expect(window.notes.toggleDiffNote).not.toHaveBeenCalled(); + expect(Notes.instance.toggleDiffNote).not.toHaveBeenCalled(); }); }); @@ -359,7 +358,7 @@ import 'vendor/jquery.scrollTo'; this.class.loadDiff('/foo/bar/merge_requests/1/diffs'); expect(noteLineNumId.length).toBeGreaterThan(0); - expect(window.notes.toggleDiffNote).not.toHaveBeenCalled(); + expect(Notes.instance.toggleDiffNote).not.toHaveBeenCalled(); }); }); }); @@ -393,7 +392,7 @@ import 'vendor/jquery.scrollTo'; this.class.loadDiff('/foo/bar/merge_requests/1/diffs'); expect(noteId.length).toBeGreaterThan(0); - expect(window.notes.toggleDiffNote).toHaveBeenCalledWith({ + expect(Notes.instance.toggleDiffNote).toHaveBeenCalledWith({ target: jasmine.any(Object), lineType: 'new', forceShow: true, @@ -404,7 +403,7 @@ import 'vendor/jquery.scrollTo'; spyOn(urlUtils, 'getLocationHash').and.returnValue('note_something-that-does-not-exist'); this.class.loadDiff('/foo/bar/merge_requests/1/diffs'); - expect(window.notes.toggleDiffNote).not.toHaveBeenCalled(); + expect(Notes.instance.toggleDiffNote).not.toHaveBeenCalled(); }); }); @@ -414,7 +413,7 @@ import 'vendor/jquery.scrollTo'; this.class.loadDiff('/foo/bar/merge_requests/1/diffs'); expect(noteLineNumId.length).toBeGreaterThan(0); - expect(window.notes.toggleDiffNote).not.toHaveBeenCalled(); + expect(Notes.instance.toggleDiffNote).not.toHaveBeenCalled(); }); }); }); diff --git a/spec/javascripts/notes_spec.js b/spec/javascripts/notes_spec.js index e09b8dc7fc5..167f074fb9b 100644 --- a/spec/javascripts/notes_spec.js +++ b/spec/javascripts/notes_spec.js @@ -1,12 +1,10 @@ /* eslint-disable space-before-function-paren, no-unused-expressions, no-var, object-shorthand, comma-dangle, max-len */ -/* global Notes */ - import * as urlUtils from '~/lib/utils/url_utility'; import 'autosize'; import '~/gl_form'; import '~/lib/utils/text_utility'; import '~/render_gfm'; -import '~/notes'; +import Notes from '~/notes'; (function() { window.gon || (window.gon = {}); From 9616e70a08b3900ae3a605ffbbe7ef5ebc763068 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Sat, 16 Dec 2017 17:28:59 +0100 Subject: [PATCH 14/50] Allow environment_scope in cluster controllers --- app/controllers/projects/clusters/gcp_controller.rb | 1 + app/controllers/projects/clusters/user_controller.rb | 1 + app/controllers/projects/clusters_controller.rb | 2 ++ 3 files changed, 4 insertions(+) diff --git a/app/controllers/projects/clusters/gcp_controller.rb b/app/controllers/projects/clusters/gcp_controller.rb index b64f7a2a6bd..d3b9d8a9bbc 100644 --- a/app/controllers/projects/clusters/gcp_controller.rb +++ b/app/controllers/projects/clusters/gcp_controller.rb @@ -39,6 +39,7 @@ class Projects::Clusters::GcpController < Projects::ApplicationController params.require(:cluster).permit( :enabled, :name, + :environment_scope, provider_gcp_attributes: [ :gcp_project_id, :zone, diff --git a/app/controllers/projects/clusters/user_controller.rb b/app/controllers/projects/clusters/user_controller.rb index d7678512073..d0db64b2fa9 100644 --- a/app/controllers/projects/clusters/user_controller.rb +++ b/app/controllers/projects/clusters/user_controller.rb @@ -26,6 +26,7 @@ class Projects::Clusters::UserController < Projects::ApplicationController params.require(:cluster).permit( :enabled, :name, + :environment_scope, platform_kubernetes_attributes: [ :namespace, :api_url, diff --git a/app/controllers/projects/clusters_controller.rb b/app/controllers/projects/clusters_controller.rb index 4a7879db313..1dc7f1b3a7f 100644 --- a/app/controllers/projects/clusters_controller.rb +++ b/app/controllers/projects/clusters_controller.rb @@ -87,6 +87,7 @@ class Projects::ClustersController < Projects::ApplicationController if cluster.managed? params.require(:cluster).permit( :enabled, + :environment_scope, platform_kubernetes_attributes: [ :namespace ] @@ -95,6 +96,7 @@ class Projects::ClustersController < Projects::ApplicationController params.require(:cluster).permit( :enabled, :name, + :environment_scope, platform_kubernetes_attributes: [ :api_url, :token, From 8a13ef41edce213a7733399bbcfea5bdce77d269 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Sat, 16 Dec 2017 17:42:59 +0100 Subject: [PATCH 15/50] Add environment scope field to cluster forms --- app/views/projects/clusters/_enabled.html.haml | 2 +- app/views/projects/clusters/gcp/_form.html.haml | 3 +++ app/views/projects/clusters/gcp/_show.html.haml | 1 + app/views/projects/clusters/user/_form.html.haml | 3 +++ 4 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/views/projects/clusters/_enabled.html.haml b/app/views/projects/clusters/_enabled.html.haml index 547b3c8446f..07f07dfbb0c 100644 --- a/app/views/projects/clusters/_enabled.html.haml +++ b/app/views/projects/clusters/_enabled.html.haml @@ -14,4 +14,4 @@ - if can?(current_user, :update_cluster, @cluster) .form-group - = field.submit _('Save'), class: 'btn btn-success' + = field.submit _('Save changes'), class: 'btn btn-success' diff --git a/app/views/projects/clusters/gcp/_form.html.haml b/app/views/projects/clusters/gcp/_form.html.haml index 0f6bae97571..4dcaf568d46 100644 --- a/app/views/projects/clusters/gcp/_form.html.haml +++ b/app/views/projects/clusters/gcp/_form.html.haml @@ -7,6 +7,9 @@ .form-group = field.label :name, s_('ClusterIntegration|Cluster name') = field.text_field :name, class: 'form-control', placeholder: s_('ClusterIntegration|Cluster name') + .form-group + = field.label :environment_scope, s_('ClusterIntegration|Environment scope') + = field.text_field :environment_scope, class: 'form-control js-select-on-focus', disabled: true, placeholder: s_('ClusterIntegration|Environment scope') = field.fields_for :provider_gcp, @cluster.provider_gcp do |provider_gcp_field| .form-group diff --git a/app/views/projects/clusters/gcp/_show.html.haml b/app/views/projects/clusters/gcp/_show.html.haml index 3fa9f69708a..f3122a1bf47 100644 --- a/app/views/projects/clusters/gcp/_show.html.haml +++ b/app/views/projects/clusters/gcp/_show.html.haml @@ -8,6 +8,7 @@ = form_for @cluster, url: namespace_project_cluster_path(@project.namespace, @project, @cluster), as: :cluster do |field| = form_errors(@cluster) + = field.fields_for :platform_kubernetes, @cluster.platform_kubernetes do |platform_kubernetes_field| .form-group = platform_kubernetes_field.label :api_url, s_('ClusterIntegration|API URL') diff --git a/app/views/projects/clusters/user/_form.html.haml b/app/views/projects/clusters/user/_form.html.haml index 4a9bd5186c6..f195b9815bd 100644 --- a/app/views/projects/clusters/user/_form.html.haml +++ b/app/views/projects/clusters/user/_form.html.haml @@ -3,6 +3,9 @@ .form-group = field.label :name, s_('ClusterIntegration|Cluster name') = field.text_field :name, class: 'form-control', placeholder: s_('ClusterIntegration|Cluster name') + .form-group + = field.label :environment_scope, s_('ClusterIntegration|Environment scope') + = field.text_field :environment_scope, class: 'form-control js-select-on-focus', disabled: true, placeholder: s_('ClusterIntegration|Environment scope') = field.fields_for :platform_kubernetes, @cluster.platform_kubernetes do |platform_kubernetes_field| .form-group From 56f719f86767a155fa721ff35f8614359092276d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Sat, 16 Dec 2017 20:21:11 +0100 Subject: [PATCH 16/50] Revert save button caption change --- app/views/projects/clusters/_enabled.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/clusters/_enabled.html.haml b/app/views/projects/clusters/_enabled.html.haml index 07f07dfbb0c..547b3c8446f 100644 --- a/app/views/projects/clusters/_enabled.html.haml +++ b/app/views/projects/clusters/_enabled.html.haml @@ -14,4 +14,4 @@ - if can?(current_user, :update_cluster, @cluster) .form-group - = field.submit _('Save changes'), class: 'btn btn-success' + = field.submit _('Save'), class: 'btn btn-success' From cc759465927e962b8aee3cf614aaead780fc1164 Mon Sep 17 00:00:00 2001 From: Jacopo Date: Sat, 16 Dec 2017 16:01:02 +0100 Subject: [PATCH 17/50] List of avatars should never show +1 The list of avatars should never show +1. If you have +1, that means you can use that space to show that person's avatar, for that reason should always start with +2. --- app/views/shared/issuable/_assignees.html.haml | 18 +++++++----------- .../unreleased/39298-list-of-avatars-2.yml | 5 +++++ 2 files changed, 12 insertions(+), 11 deletions(-) create mode 100644 changelogs/unreleased/39298-list-of-avatars-2.yml diff --git a/app/views/shared/issuable/_assignees.html.haml b/app/views/shared/issuable/_assignees.html.haml index 217af7c9fac..fc86f855865 100644 --- a/app/views/shared/issuable/_assignees.html.haml +++ b/app/views/shared/issuable/_assignees.html.haml @@ -1,14 +1,10 @@ -- max_render = 3 -- max = [max_render, issue.assignees.length].min +- max_render = 4 +- assignees_rendering_overflow = issue.assignees.size > max_render +- render_count = assignees_rendering_overflow ? max_render - 1 : max_render +- more_assignees_count = issue.assignees.size - render_count -- issue.assignees.take(max).each do |assignee| +- issue.assignees.take(render_count).each do |assignee| = link_to_member(@project, assignee, name: false, title: "Assigned to :name") -- if issue.assignees.length > max_render - - counter = issue.assignees.length - max_render - - %span{ class: 'avatar-counter has-tooltip', data: { container: 'body', placement: 'bottom', 'line-type' => 'old', 'original-title' => "+#{counter} more assignees" } } - - if counter < 99 - = "+#{counter}" - - else - 99+ +- if more_assignees_count.positive? + %span{ class: 'avatar-counter has-tooltip', data: { container: 'body', placement: 'bottom', 'line-type' => 'old', 'original-title' => "+#{more_assignees_count} more assignees" } } +#{more_assignees_count} diff --git a/changelogs/unreleased/39298-list-of-avatars-2.yml b/changelogs/unreleased/39298-list-of-avatars-2.yml new file mode 100644 index 00000000000..e2095561c0e --- /dev/null +++ b/changelogs/unreleased/39298-list-of-avatars-2.yml @@ -0,0 +1,5 @@ +--- +title: List of avatars should never show +1 +merge_request: 15972 +author: Jacopo Beschi @jacopo-beschi +type: added From c7807741a47dfc2024817eddf289fb9d8211994d Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Fri, 15 Dec 2017 12:20:42 +0000 Subject: [PATCH 18/50] Document LFS integrity check and how to disable it --- doc/workflow/lfs/manage_large_binaries_with_git_lfs.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/doc/workflow/lfs/manage_large_binaries_with_git_lfs.md b/doc/workflow/lfs/manage_large_binaries_with_git_lfs.md index 6adde447975..195285f9157 100644 --- a/doc/workflow/lfs/manage_large_binaries_with_git_lfs.md +++ b/doc/workflow/lfs/manage_large_binaries_with_git_lfs.md @@ -163,3 +163,11 @@ For Windows, you can use `wincred` or Microsoft's [Git Credential Manager for Wi More details about various methods of storing the user credentials can be found on [Git Credential Storage documentation](https://git-scm.com/book/en/v2/Git-Tools-Credential-Storage). + +### LFS objects are missing on push + +GitLab checks files to detect LFS pointers on push. If LFS pointers are detected, GitLab tries to verify that those files already exist in LFS on GitLab. + +Verify that LFS in installed locally and consider a manual push with `git lfs push --all`. + +If you are storing LFS files outside of GitLab you can disable LFS on the project by settting `lfs_enabled: false` with the [projets api](../../api/projects.md#edit-project). From ef454f68e837e4e7360fe1518686dd56adbbb0a9 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 8 Dec 2017 12:17:22 +0000 Subject: [PATCH 19/50] Reset todo counters when the target is deleted When the target is deleted, todos are destroyed, but we did not reset the todo cache for users with todos on the deleted target. This would only update after the next time the todo cache was updated for that user. --- app/controllers/concerns/issuable_actions.rb | 1 - app/services/issuable/destroy_service.rb | 6 +++-- app/services/notes/destroy_service.rb | 4 +++- app/services/todo_service.rb | 16 +++++++++---- ...ows-notification-without-having-a-todo.yml | 5 ++++ .../projects/issues_controller_spec.rb | 2 +- .../merge_requests_controller_spec.rb | 2 +- .../services/issuable/destroy_service_spec.rb | 16 ++++++++++++- spec/services/notes/destroy_service_spec.rb | 16 ++++++++++--- spec/services/todo_service_spec.rb | 23 +++++++++++++++---- 10 files changed, 73 insertions(+), 18 deletions(-) create mode 100644 changelogs/unreleased/40871-todo-notification-count-shows-notification-without-having-a-todo.yml diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index 744e448e8df..141c34ee5ee 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -55,7 +55,6 @@ module IssuableActions def destroy Issuable::DestroyService.new(issuable.project, current_user).execute(issuable) - TodoService.new.destroy_issuable(issuable, current_user) name = issuable.human_class_name flash[:notice] = "The #{name} was successfully deleted." diff --git a/app/services/issuable/destroy_service.rb b/app/services/issuable/destroy_service.rb index 0610b401213..7197a426a72 100644 --- a/app/services/issuable/destroy_service.rb +++ b/app/services/issuable/destroy_service.rb @@ -1,8 +1,10 @@ module Issuable class DestroyService < IssuableBaseService def execute(issuable) - if issuable.destroy - issuable.update_project_counter_caches + TodoService.new.destroy_target(issuable) do |issuable| + if issuable.destroy + issuable.update_project_counter_caches + end end end end diff --git a/app/services/notes/destroy_service.rb b/app/services/notes/destroy_service.rb index b819bd17039..fb78420d324 100644 --- a/app/services/notes/destroy_service.rb +++ b/app/services/notes/destroy_service.rb @@ -1,7 +1,9 @@ module Notes class DestroyService < BaseService def execute(note) - note.destroy + TodoService.new.destroy_target(note) do |note| + note.destroy + end end end end diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index 575853fd66b..c2ca404b179 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -31,12 +31,20 @@ class TodoService mark_pending_todos_as_done(issue, current_user) end - # When we destroy an issuable we should: + # When we destroy a todo target we should: # - # * refresh the todos count cache for the current user + # * refresh the todos count cache for all users with todos on the target # - def destroy_issuable(issuable, user) - user.update_todos_count_cache + # This needs to yield back to the caller to destroy the target, because it + # collects the todo users before the todos themselves are deleted, then + # updates the todo counts for those users. + # + def destroy_target(target) + todo_users = User.where(id: target.todos.pending.select(:user_id)).to_a + + yield target + + todo_users.each(&:update_todos_count_cache) end # When we reassign an issue we should: diff --git a/changelogs/unreleased/40871-todo-notification-count-shows-notification-without-having-a-todo.yml b/changelogs/unreleased/40871-todo-notification-count-shows-notification-without-having-a-todo.yml new file mode 100644 index 00000000000..ee196629def --- /dev/null +++ b/changelogs/unreleased/40871-todo-notification-count-shows-notification-without-having-a-todo.yml @@ -0,0 +1,5 @@ +--- +title: Reset todo counters when the target is deleted +merge_request: 15807 +author: +type: fixed diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 4dbbaecdd6d..b7447f654c4 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -861,7 +861,7 @@ describe Projects::IssuesController do end it 'delegates the update of the todos count cache to TodoService' do - expect_any_instance_of(TodoService).to receive(:destroy_issuable).with(issue, owner).once + expect_any_instance_of(TodoService).to receive(:destroy_target).with(issue).once delete :destroy, namespace_id: project.namespace, project_id: project, id: issue.iid end diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 51d5d6a52b3..97a5018d714 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -468,7 +468,7 @@ describe Projects::MergeRequestsController do end it 'delegates the update of the todos count cache to TodoService' do - expect_any_instance_of(TodoService).to receive(:destroy_issuable).with(merge_request, owner).once + expect_any_instance_of(TodoService).to receive(:destroy_target).with(merge_request).once delete :destroy, namespace_id: project.namespace, project_id: project, id: merge_request.iid end diff --git a/spec/services/issuable/destroy_service_spec.rb b/spec/services/issuable/destroy_service_spec.rb index d74d98c6079..0a3647a814f 100644 --- a/spec/services/issuable/destroy_service_spec.rb +++ b/spec/services/issuable/destroy_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Issuable::DestroyService do let(:user) { create(:user) } - let(:project) { create(:project) } + let(:project) { create(:project, :public) } subject(:service) { described_class.new(project, user) } @@ -19,6 +19,13 @@ describe Issuable::DestroyService do service.execute(issue) end + + it 'updates the todo caches for users with todos on the issue' do + create(:todo, target: issue, user: user, author: user, project: project) + + expect { service.execute(issue) } + .to change { user.todos_pending_count }.from(1).to(0) + end end context 'when issuable is a merge request' do @@ -33,6 +40,13 @@ describe Issuable::DestroyService do service.execute(merge_request) end + + it 'updates the todo caches for users with todos on the merge request' do + create(:todo, target: merge_request, user: user, author: user, project: project) + + expect { service.execute(merge_request) } + .to change { user.todos_pending_count }.from(1).to(0) + end end end end diff --git a/spec/services/notes/destroy_service_spec.rb b/spec/services/notes/destroy_service_spec.rb index c9a99a43edb..64445be560e 100644 --- a/spec/services/notes/destroy_service_spec.rb +++ b/spec/services/notes/destroy_service_spec.rb @@ -1,15 +1,25 @@ require 'spec_helper' describe Notes::DestroyService do + set(:project) { create(:project, :public) } + set(:issue) { create(:issue, project: project) } + let(:user) { issue.author } + describe '#execute' do it 'deletes a note' do - project = create(:project) - issue = create(:issue, project: project) note = create(:note, project: project, noteable: issue) - described_class.new(project, note.author).execute(note) + described_class.new(project, user).execute(note) expect(project.issues.find(issue.id).notes).not_to include(note) end + + it 'updates the todo counts for users with todos for the note' do + note = create(:note, project: project, noteable: issue) + create(:todo, note: note, target: issue, user: user, author: user, project: project) + + expect { described_class.new(project, user).execute(note) } + .to change { user.todos_pending_count }.from(1).to(0) + end end end diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index dc2673abc73..88013acae0a 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -248,11 +248,26 @@ describe TodoService do end end - describe '#destroy_issuable' do - it 'refresh the todos count cache for the user' do - expect(john_doe).to receive(:update_todos_count_cache).and_call_original + describe '#destroy_target' do + it 'refreshes the todos count cache for users with todos on the target' do + create(:todo, target: issue, user: john_doe, author: john_doe, project: issue.project) - service.destroy_issuable(issue, john_doe) + expect_any_instance_of(User).to receive(:update_todos_count_cache).and_call_original + + service.destroy_target(issue) { } + end + + it 'does not refresh the todos count cache for users with only done todos on the target' do + create(:todo, :done, target: issue, user: john_doe, author: john_doe, project: issue.project) + + expect_any_instance_of(User).not_to receive(:update_todos_count_cache) + + service.destroy_target(issue) { } + end + + it 'yields the target to the caller' do + expect { |b| service.destroy_target(issue, &b) } + .to yield_with_args(issue) end end From dd66c6a2d8fa7f84f200e150791dc4a3953111d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 18 Dec 2017 14:13:57 +0100 Subject: [PATCH 20/50] Use helper for feature check in cluster partials --- app/helpers/clusters_helper.rb | 5 +++++ app/views/projects/clusters/gcp/_form.html.haml | 2 +- app/views/projects/clusters/gcp/_show.html.haml | 4 ++++ app/views/projects/clusters/user/_form.html.haml | 2 +- app/views/projects/clusters/user/_show.html.haml | 4 ++++ 5 files changed, 15 insertions(+), 2 deletions(-) create mode 100644 app/helpers/clusters_helper.rb diff --git a/app/helpers/clusters_helper.rb b/app/helpers/clusters_helper.rb new file mode 100644 index 00000000000..60704af3497 --- /dev/null +++ b/app/helpers/clusters_helper.rb @@ -0,0 +1,5 @@ +module ClustersHelper + def has_multiple_clusters?(project) + project.feature_available?(:multiple_clusters) + end +end diff --git a/app/views/projects/clusters/gcp/_form.html.haml b/app/views/projects/clusters/gcp/_form.html.haml index 4dcaf568d46..21754a561ac 100644 --- a/app/views/projects/clusters/gcp/_form.html.haml +++ b/app/views/projects/clusters/gcp/_form.html.haml @@ -9,7 +9,7 @@ = field.text_field :name, class: 'form-control', placeholder: s_('ClusterIntegration|Cluster name') .form-group = field.label :environment_scope, s_('ClusterIntegration|Environment scope') - = field.text_field :environment_scope, class: 'form-control js-select-on-focus', disabled: true, placeholder: s_('ClusterIntegration|Environment scope') + = field.text_field :environment_scope, class: 'form-control', readonly: has_multiple_clusters?(@project), placeholder: s_('ClusterIntegration|Environment scope') = field.fields_for :provider_gcp, @cluster.provider_gcp do |provider_gcp_field| .form-group diff --git a/app/views/projects/clusters/gcp/_show.html.haml b/app/views/projects/clusters/gcp/_show.html.haml index f3122a1bf47..22a6324ac39 100644 --- a/app/views/projects/clusters/gcp/_show.html.haml +++ b/app/views/projects/clusters/gcp/_show.html.haml @@ -9,6 +9,10 @@ = form_for @cluster, url: namespace_project_cluster_path(@project.namespace, @project, @cluster), as: :cluster do |field| = form_errors(@cluster) + .form-group + = field.label :environment_scope, s_('ClusterIntegration|Environment scope') + = field.text_field :environment_scope, class: 'form-control js-select-on-focus', readonly: has_multiple_clusters?(@project), placeholder: s_('ClusterIntegration|Environment scope') + = field.fields_for :platform_kubernetes, @cluster.platform_kubernetes do |platform_kubernetes_field| .form-group = platform_kubernetes_field.label :api_url, s_('ClusterIntegration|API URL') diff --git a/app/views/projects/clusters/user/_form.html.haml b/app/views/projects/clusters/user/_form.html.haml index f195b9815bd..ff575ee2671 100644 --- a/app/views/projects/clusters/user/_form.html.haml +++ b/app/views/projects/clusters/user/_form.html.haml @@ -5,7 +5,7 @@ = field.text_field :name, class: 'form-control', placeholder: s_('ClusterIntegration|Cluster name') .form-group = field.label :environment_scope, s_('ClusterIntegration|Environment scope') - = field.text_field :environment_scope, class: 'form-control js-select-on-focus', disabled: true, placeholder: s_('ClusterIntegration|Environment scope') + = field.text_field :environment_scope, class: 'form-control', readonly: has_multiple_clusters?(@project), placeholder: s_('ClusterIntegration|Environment scope') = field.fields_for :platform_kubernetes, @cluster.platform_kubernetes do |platform_kubernetes_field| .form-group diff --git a/app/views/projects/clusters/user/_show.html.haml b/app/views/projects/clusters/user/_show.html.haml index 5931e0b7f17..b07a98e9ef4 100644 --- a/app/views/projects/clusters/user/_show.html.haml +++ b/app/views/projects/clusters/user/_show.html.haml @@ -4,6 +4,10 @@ = field.label :name, s_('ClusterIntegration|Cluster name') = field.text_field :name, class: 'form-control', placeholder: s_('ClusterIntegration|Cluster name') + .form-group + = field.label :environment_scope, s_('ClusterIntegration|Environment scope') + = field.text_field :environment_scope, class: 'form-control js-select-on-focus', readonly: has_multiple_clusters?(@project), placeholder: s_('ClusterIntegration|Environment scope') + = field.fields_for :platform_kubernetes, @cluster.platform_kubernetes do |platform_kubernetes_field| .form-group = platform_kubernetes_field.label :api_url, s_('ClusterIntegration|API URL') From 2b45ae0933e29631f7ed67e5b855e22d5cb32ed5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 18 Dec 2017 14:35:08 +0100 Subject: [PATCH 21/50] Hardcode clusters helper in CE --- app/helpers/clusters_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/helpers/clusters_helper.rb b/app/helpers/clusters_helper.rb index 60704af3497..7e4eb06b99d 100644 --- a/app/helpers/clusters_helper.rb +++ b/app/helpers/clusters_helper.rb @@ -1,5 +1,5 @@ module ClustersHelper def has_multiple_clusters?(project) - project.feature_available?(:multiple_clusters) + false end end From c77083a4938b42e1901a4cb69106a39a04f5c30d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Mon, 18 Dec 2017 10:21:33 -0500 Subject: [PATCH 22/50] fix the commit diff discussion sending the wrong url it should now send you to the merge request diff path scoped to the commit. --- app/models/diff_discussion.rb | 19 ++++++++++++++----- spec/helpers/notes_helper_spec.rb | 14 ++++++++++++-- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb index 4a65738214b..758a26d3c70 100644 --- a/app/models/diff_discussion.rb +++ b/app/models/diff_discussion.rb @@ -22,12 +22,10 @@ class DiffDiscussion < Discussion def merge_request_version_params return unless for_merge_request? - return {} if active? - if on_merge_request_commit? - { commit_id: commit_id } - else - noteable.version_params_for(position.diff_refs) + version_params do |params| + params[:commit_id] = commit_id if on_merge_request_commit? + params end end @@ -37,4 +35,15 @@ class DiffDiscussion < Discussion position: position.to_json ) end + + private + + def version_params + params = if active? + {} + else + noteable.version_params_for(position.diff_refs) + end + yield params + end end diff --git a/spec/helpers/notes_helper_spec.rb b/spec/helpers/notes_helper_spec.rb index cd15e27b497..36a44f8567a 100644 --- a/spec/helpers/notes_helper_spec.rb +++ b/spec/helpers/notes_helper_spec.rb @@ -41,6 +41,7 @@ describe NotesHelper do describe '#discussion_path' do let(:project) { create(:project, :repository) } + let(:anchor) { discussion.line_code } context 'for a merge request discusion' do let(:merge_request) { create(:merge_request, source_project: project, target_project: project, importing: true) } @@ -151,6 +152,15 @@ describe NotesHelper do expect(helper.discussion_path(discussion)).to be_nil end end + + context 'for a contextual commit discussion' do + let(:commit) { merge_request.commits.last } + let(:discussion) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, commit_id: commit.id).to_discussion } + + it 'returns the merge request diff discussion scoped in the commit' do + expect(helper.discussion_path(discussion)).to eq(diffs_project_merge_request_path(project, merge_request, commit_id: commit.id, anchor: anchor)) + end + end end context 'for a commit discussion' do @@ -160,7 +170,7 @@ describe NotesHelper do let(:discussion) { create(:diff_note_on_commit, project: project).to_discussion } it 'returns the commit path with the line code' do - expect(helper.discussion_path(discussion)).to eq(project_commit_path(project, commit, anchor: discussion.line_code)) + expect(helper.discussion_path(discussion)).to eq(project_commit_path(project, commit, anchor: anchor)) end end @@ -168,7 +178,7 @@ describe NotesHelper do let(:discussion) { create(:legacy_diff_note_on_commit, project: project).to_discussion } it 'returns the commit path with the line code' do - expect(helper.discussion_path(discussion)).to eq(project_commit_path(project, commit, anchor: discussion.line_code)) + expect(helper.discussion_path(discussion)).to eq(project_commit_path(project, commit, anchor: anchor)) end end From c94f417dfabb6f1e6b096a6019884e039579b38a Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 18 Dec 2017 23:42:31 +0800 Subject: [PATCH 23/50] Use queue_namespace rather than enqueue_in --- app/workers/run_pipeline_schedule_worker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/workers/run_pipeline_schedule_worker.rb b/app/workers/run_pipeline_schedule_worker.rb index 7725ad319a3..8f5138fc873 100644 --- a/app/workers/run_pipeline_schedule_worker.rb +++ b/app/workers/run_pipeline_schedule_worker.rb @@ -2,7 +2,7 @@ class RunPipelineScheduleWorker include ApplicationWorker include PipelineQueue - enqueue_in group: :creation + queue_namespace :pipeline_creation def perform(schedule_id, user_id) schedule = Ci::PipelineSchedule.find_by(id: schedule_id) From e4b72248c243d66c19e300ed88532c5c795877cd Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Mon, 18 Dec 2017 15:54:44 +0000 Subject: [PATCH 24/50] Only render signout screen when user is signed out --- .../shared/empty_states/_issues.html.haml | 13 +- spec/features/issues_spec.rb | 1238 +++++++++-------- 2 files changed, 638 insertions(+), 613 deletions(-) diff --git a/app/views/shared/empty_states/_issues.html.haml b/app/views/shared/empty_states/_issues.html.haml index e039a73cd3b..62437f5fc9d 100644 --- a/app/views/shared/empty_states/_issues.html.haml +++ b/app/views/shared/empty_states/_issues.html.haml @@ -8,16 +8,17 @@ = image_tag 'illustrations/issues.svg' .col-xs-12 .text-content - - if has_button && current_user + - if current_user %h4 = _("The Issue Tracker is the place to add things that need to be improved or solved in a project") %p = _("Issues can be bugs, tasks or ideas to be discussed. Also, issues are searchable and filterable.") - .text-center - - if project_select_button - = render 'shared/new_project_item_select', path: 'issues/new', label: 'New issue', type: :issues - - else - = link_to 'New issue', button_path, class: 'btn btn-success', title: 'New issue', id: 'new_issue_link' + - if has_button + .text-center + - if project_select_button + = render 'shared/new_project_item_select', path: 'issues/new', label: 'New issue', type: :issues + - else + = link_to 'New issue', button_path, class: 'btn btn-success', title: 'New issue', id: 'new_issue_link' - else %h4.text-center= _("There are no issues to show") %p diff --git a/spec/features/issues_spec.rb b/spec/features/issues_spec.rb index 852d9e368aa..d1ff057a0c6 100644 --- a/spec/features/issues_spec.rb +++ b/spec/features/issues_spec.rb @@ -8,729 +8,753 @@ describe 'Issues' do let(:user) { create(:user) } let(:project) { create(:project, :public) } - before do - sign_in(user) - user2 = create(:user) + describe 'while user is signed out' do + describe 'empty state' do + it 'user sees empty state' do + visit project_issues_path(project) - project.team << [[user, user2], :developer] + expect(page).to have_content('Register / Sign In') + expect(page).to have_content('The Issue Tracker is the place to add things that need to be improved or solved in a project.') + expect(page).to have_content('You can register or sign in to create issues for this project.') + end + end end - describe 'Edit issue' do - let!(:issue) do - create(:issue, - author: user, - assignees: [user], - project: project) - end - + describe 'while user is signed in' do before do - visit edit_project_issue_path(project, issue) - find('.js-zen-enter').click + sign_in(user) + user2 = create(:user) + + project.team << [[user, user2], :developer] end - it 'opens new issue popup' do - expect(page).to have_content("Issue ##{issue.iid}") - end - end + describe 'empty state' do + it 'user sees empty state' do + visit project_issues_path(project) - describe 'Editing issue assignee' do - let!(:issue) do - create(:issue, - author: user, - assignees: [user], - project: project) - end - - it 'allows user to select unassigned', :js do - visit edit_project_issue_path(project, issue) - - expect(page).to have_content "Assignee #{user.name}" - - first('.js-user-search').click - click_link 'Unassigned' - - click_button 'Save changes' - - page.within('.assignee') do - expect(page).to have_content 'No assignee - assign yourself' - end - - expect(issue.reload.assignees).to be_empty - end - end - - describe 'due date', :js do - context 'on new form' do - before do - visit new_project_issue_path(project) - end - - it 'saves with due date' do - date = Date.today.at_beginning_of_month - - fill_in 'issue_title', with: 'bug 345' - fill_in 'issue_description', with: 'bug description' - find('#issuable-due-date').click - - page.within '.pika-single' do - click_button date.day - end - - expect(find('#issuable-due-date').value).to eq date.to_s - - click_button 'Submit issue' - - page.within '.issuable-sidebar' do - expect(page).to have_content date.to_s(:medium) - end + expect(page).to have_content('The Issue Tracker is the place to add things that need to be improved or solved in a project') + expect(page).to have_content('Issues can be bugs, tasks or ideas to be discussed. Also, issues are searchable and filterable.') + expect(page).to have_content('New issue') end end - context 'on edit form' do - let(:issue) { create(:issue, author: user, project: project, due_date: Date.today.at_beginning_of_month.to_s) } - - before do - visit edit_project_issue_path(project, issue) - end - - it 'saves with due date' do - date = Date.today.at_beginning_of_month - - expect(find('#issuable-due-date').value).to eq date.to_s - - date = date.tomorrow - - fill_in 'issue_title', with: 'bug 345' - fill_in 'issue_description', with: 'bug description' - find('#issuable-due-date').click - - page.within '.pika-single' do - click_button date.day - end - - expect(find('#issuable-due-date').value).to eq date.to_s - - click_button 'Save changes' - - page.within '.issuable-sidebar' do - expect(page).to have_content date.to_s(:medium) - end - end - - it 'warns about version conflict' do - issue.update(title: "New title") - - fill_in 'issue_title', with: 'bug 345' - fill_in 'issue_description', with: 'bug description' - - click_button 'Save changes' - - expect(page).to have_content 'Someone edited the issue the same time you did' - end - end - end - - describe 'Issue info' do - it 'links to current issue in breadcrubs' do - issue = create(:issue, project: project) - - visit project_issue_path(project, issue) - - expect(find('.breadcrumbs-sub-title a')[:href]).to end_with(issue_path(issue)) - end - - it 'excludes award_emoji from comment count' do - issue = create(:issue, author: user, assignees: [user], project: project, title: 'foobar') - create(:award_emoji, awardable: issue) - - visit project_issues_path(project, assignee_id: user.id) - - expect(page).to have_content 'foobar' - expect(page.all('.no-comments').first.text).to eq "0" - end - end - - describe 'Filter issue' do - before do - %w(foobar barbaz gitlab).each do |title| + describe 'Edit issue' do + let!(:issue) do create(:issue, author: user, assignees: [user], - project: project, - title: title) + project: project) end - @issue = Issue.find_by(title: 'foobar') - @issue.milestone = create(:milestone, project: project) - @issue.assignees = [] - @issue.save - end - - let(:issue) { @issue } - - it 'allows filtering by issues with no specified assignee' do - visit project_issues_path(project, assignee_id: IssuableFinder::NONE) - - expect(page).to have_content 'foobar' - expect(page).not_to have_content 'barbaz' - expect(page).not_to have_content 'gitlab' - end - - it 'allows filtering by a specified assignee' do - visit project_issues_path(project, assignee_id: user.id) - - expect(page).not_to have_content 'foobar' - expect(page).to have_content 'barbaz' - expect(page).to have_content 'gitlab' - end - end - - describe 'filter issue' do - titles = %w[foo bar baz] - titles.each_with_index do |title, index| - let!(title.to_sym) do - create(:issue, title: title, - project: project, - created_at: Time.now - (index * 60)) - end - end - let(:newer_due_milestone) { create(:milestone, due_date: '2013-12-11') } - let(:later_due_milestone) { create(:milestone, due_date: '2013-12-12') } - - it 'sorts by newest' do - visit project_issues_path(project, sort: sort_value_created_date) - - expect(first_issue).to include('foo') - expect(last_issue).to include('baz') - end - - it 'sorts by most recently updated' do - baz.updated_at = Time.now + 100 - baz.save - visit project_issues_path(project, sort: sort_value_recently_updated) - - expect(first_issue).to include('baz') - end - - describe 'sorting by due date' do before do - foo.update(due_date: 1.day.from_now) - bar.update(due_date: 6.days.from_now) + visit edit_project_issue_path(project, issue) + find('.js-zen-enter').click end - it 'sorts by due date' do - visit project_issues_path(project, sort: sort_value_due_date) + it 'opens new issue popup' do + expect(page).to have_content("Issue ##{issue.iid}") + end + end - expect(first_issue).to include('foo') + describe 'Editing issue assignee' do + let!(:issue) do + create(:issue, + author: user, + assignees: [user], + project: project) end - it 'sorts by due date by excluding nil due dates' do - bar.update(due_date: nil) + it 'allows user to select unassigned', :js do + visit edit_project_issue_path(project, issue) - visit project_issues_path(project, sort: sort_value_due_date) + expect(page).to have_content "Assignee #{user.name}" - expect(first_issue).to include('foo') + first('.js-user-search').click + click_link 'Unassigned' + + click_button 'Save changes' + + page.within('.assignee') do + expect(page).to have_content 'No assignee - assign yourself' + end + + expect(issue.reload.assignees).to be_empty + end + end + + describe 'due date', :js do + context 'on new form' do + before do + visit new_project_issue_path(project) + end + + it 'saves with due date' do + date = Date.today.at_beginning_of_month + + fill_in 'issue_title', with: 'bug 345' + fill_in 'issue_description', with: 'bug description' + find('#issuable-due-date').click + + page.within '.pika-single' do + click_button date.day + end + + expect(find('#issuable-due-date').value).to eq date.to_s + + click_button 'Submit issue' + + page.within '.issuable-sidebar' do + expect(page).to have_content date.to_s(:medium) + end + end end - context 'with a filter on labels' do - let(:label) { create(:label, project: project) } + context 'on edit form' do + let(:issue) { create(:issue, author: user, project: project, due_date: Date.today.at_beginning_of_month.to_s) } before do - create(:label_link, label: label, target: foo) + visit edit_project_issue_path(project, issue) end - it 'sorts by least recently due date by excluding nil due dates' do - bar.update(due_date: nil) + it 'saves with due date' do + date = Date.today.at_beginning_of_month - visit project_issues_path(project, label_names: [label.name], sort: sort_value_due_date_later) + expect(find('#issuable-due-date').value).to eq date.to_s - expect(first_issue).to include('foo') + date = date.tomorrow + + fill_in 'issue_title', with: 'bug 345' + fill_in 'issue_description', with: 'bug description' + find('#issuable-due-date').click + + page.within '.pika-single' do + click_button date.day + end + + expect(find('#issuable-due-date').value).to eq date.to_s + + click_button 'Save changes' + + page.within '.issuable-sidebar' do + expect(page).to have_content date.to_s(:medium) + end + end + + it 'warns about version conflict' do + issue.update(title: "New title") + + fill_in 'issue_title', with: 'bug 345' + fill_in 'issue_description', with: 'bug description' + + click_button 'Save changes' + + expect(page).to have_content 'Someone edited the issue the same time you did' end end end - describe 'filtering by due date' do - before do - foo.update(due_date: 1.day.from_now) - bar.update(due_date: 6.days.from_now) + describe 'Issue info' do + it 'links to current issue in breadcrubs' do + issue = create(:issue, project: project) + + visit project_issue_path(project, issue) + + expect(find('.breadcrumbs-sub-title a')[:href]).to end_with(issue_path(issue)) end - it 'filters by none' do - visit project_issues_path(project, due_date: Issue::NoDueDate.name) + it 'excludes award_emoji from comment count' do + issue = create(:issue, author: user, assignees: [user], project: project, title: 'foobar') + create(:award_emoji, awardable: issue) - page.within '.issues-holder' do - expect(page).not_to have_content('foo') - expect(page).not_to have_content('bar') - expect(page).to have_content('baz') - end - end + visit project_issues_path(project, assignee_id: user.id) - it 'filters by any' do - visit project_issues_path(project, due_date: Issue::AnyDueDate.name) - - page.within '.issues-holder' do - expect(page).to have_content('foo') - expect(page).to have_content('bar') - expect(page).to have_content('baz') - end - end - - it 'filters by due this week' do - foo.update(due_date: Date.today.beginning_of_week + 2.days) - bar.update(due_date: Date.today.end_of_week) - baz.update(due_date: Date.today - 8.days) - - visit project_issues_path(project, due_date: Issue::DueThisWeek.name) - - page.within '.issues-holder' do - expect(page).to have_content('foo') - expect(page).to have_content('bar') - expect(page).not_to have_content('baz') - end - end - - it 'filters by due this month' do - foo.update(due_date: Date.today.beginning_of_month + 2.days) - bar.update(due_date: Date.today.end_of_month) - baz.update(due_date: Date.today - 50.days) - - visit project_issues_path(project, due_date: Issue::DueThisMonth.name) - - page.within '.issues-holder' do - expect(page).to have_content('foo') - expect(page).to have_content('bar') - expect(page).not_to have_content('baz') - end - end - - it 'filters by overdue' do - foo.update(due_date: Date.today + 2.days) - bar.update(due_date: Date.today + 20.days) - baz.update(due_date: Date.yesterday) - - visit project_issues_path(project, due_date: Issue::Overdue.name) - - page.within '.issues-holder' do - expect(page).not_to have_content('foo') - expect(page).not_to have_content('bar') - expect(page).to have_content('baz') - end + expect(page).to have_content 'foobar' + expect(page.all('.no-comments').first.text).to eq "0" end end - describe 'sorting by milestone' do + describe 'Filter issue' do before do - foo.milestone = newer_due_milestone - foo.save - bar.milestone = later_due_milestone - bar.save + %w(foobar barbaz gitlab).each do |title| + create(:issue, + author: user, + assignees: [user], + project: project, + title: title) + end + + @issue = Issue.find_by(title: 'foobar') + @issue.milestone = create(:milestone, project: project) + @issue.assignees = [] + @issue.save end - it 'sorts by milestone' do - visit project_issues_path(project, sort: sort_value_milestone) + let(:issue) { @issue } + + it 'allows filtering by issues with no specified assignee' do + visit project_issues_path(project, assignee_id: IssuableFinder::NONE) + + expect(page).to have_content 'foobar' + expect(page).not_to have_content 'barbaz' + expect(page).not_to have_content 'gitlab' + end + + it 'allows filtering by a specified assignee' do + visit project_issues_path(project, assignee_id: user.id) + + expect(page).not_to have_content 'foobar' + expect(page).to have_content 'barbaz' + expect(page).to have_content 'gitlab' + end + end + + describe 'filter issue' do + titles = %w[foo bar baz] + titles.each_with_index do |title, index| + let!(title.to_sym) do + create(:issue, title: title, + project: project, + created_at: Time.now - (index * 60)) + end + end + let(:newer_due_milestone) { create(:milestone, due_date: '2013-12-11') } + let(:later_due_milestone) { create(:milestone, due_date: '2013-12-12') } + + it 'sorts by newest' do + visit project_issues_path(project, sort: sort_value_created_date) expect(first_issue).to include('foo') expect(last_issue).to include('baz') end - end - describe 'combine filter and sort' do - let(:user2) { create(:user) } + it 'sorts by most recently updated' do + baz.updated_at = Time.now + 100 + baz.save + visit project_issues_path(project, sort: sort_value_recently_updated) - before do - foo.assignees << user2 - foo.save - bar.assignees << user2 - bar.save + expect(first_issue).to include('baz') end - it 'sorts with a filter applied' do - visit project_issues_path(project, sort: sort_value_created_date, assignee_id: user2.id) - - expect(first_issue).to include('foo') - expect(last_issue).to include('bar') - expect(page).not_to have_content('baz') - end - end - end - - describe 'when I want to reset my incoming email token' do - let(:project1) { create(:project, namespace: user.namespace) } - let!(:issue) { create(:issue, project: project1) } - - before do - stub_incoming_email_setting(enabled: true, address: "p+%{key}@gl.ab") - project1.team << [user, :master] - visit namespace_project_issues_path(user.namespace, project1) - end - - it 'changes incoming email address token', :js do - find('.issuable-email-modal-btn').click - previous_token = find('input#issuable_email').value - find('.incoming-email-token-reset').click - - wait_for_requests - - expect(page).to have_no_field('issuable_email', with: previous_token) - new_token = project1.new_issuable_address(user.reload, 'issue') - expect(page).to have_field( - 'issuable_email', - with: new_token - ) - end - end - - describe 'update labels from issue#show', :js do - let(:issue) { create(:issue, project: project, author: user, assignees: [user]) } - let!(:label) { create(:label, project: project) } - - before do - visit project_issue_path(project, issue) - end - - it 'will not send ajax request when no data is changed' do - page.within '.labels' do - click_link 'Edit' - - find('.dropdown-menu-close', match: :first).click - - expect(page).not_to have_selector('.block-loading') - end - end - end - - describe 'update assignee from issue#show' do - let(:issue) { create(:issue, project: project, author: user, assignees: [user]) } - - context 'by authorized user' do - it 'allows user to select unassigned', :js do - visit project_issue_path(project, issue) - - page.within('.assignee') do - expect(page).to have_content "#{user.name}" - - click_link 'Edit' - click_link 'Unassigned' - first('.title').click - expect(page).to have_content 'No assignee' + describe 'sorting by due date' do + before do + foo.update(due_date: 1.day.from_now) + bar.update(due_date: 6.days.from_now) end - # wait_for_requests does not work with vue-resource at the moment - sleep 1 + it 'sorts by due date' do + visit project_issues_path(project, sort: sort_value_due_date) - expect(issue.reload.assignees).to be_empty - end - - it 'allows user to select an assignee', :js do - issue2 = create(:issue, project: project, author: user) - visit project_issue_path(project, issue2) - - page.within('.assignee') do - expect(page).to have_content "No assignee" + expect(first_issue).to include('foo') end - page.within '.assignee' do - click_link 'Edit' + it 'sorts by due date by excluding nil due dates' do + bar.update(due_date: nil) + + visit project_issues_path(project, sort: sort_value_due_date) + + expect(first_issue).to include('foo') end - page.within '.dropdown-menu-user' do - click_link user.name - end + context 'with a filter on labels' do + let(:label) { create(:label, project: project) } - page.within('.assignee') do - expect(page).to have_content user.name - end - end - - it 'allows user to unselect themselves', :js do - issue2 = create(:issue, project: project, author: user) - visit project_issue_path(project, issue2) - - page.within '.assignee' do - click_link 'Edit' - click_link user.name - - page.within '.value .author' do - expect(page).to have_content user.name + before do + create(:label_link, label: label, target: foo) end - click_link 'Edit' - click_link user.name + it 'sorts by least recently due date by excluding nil due dates' do + bar.update(due_date: nil) - page.within '.value .assign-yourself' do - expect(page).to have_content "No assignee" + visit project_issues_path(project, label_names: [label.name], sort: sort_value_due_date_later) + + expect(first_issue).to include('foo') end end end - end - context 'by unauthorized user' do - let(:guest) { create(:user) } - - before do - project.team << [[guest], :guest] - end - - it 'shows assignee text', :js do - sign_out(:user) - sign_in(guest) - - visit project_issue_path(project, issue) - expect(page).to have_content issue.assignees.first.name - end - end - end - - describe 'update milestone from issue#show' do - let!(:issue) { create(:issue, project: project, author: user) } - let!(:milestone) { create(:milestone, project: project) } - - context 'by authorized user' do - it 'allows user to select unassigned', :js do - visit project_issue_path(project, issue) - - page.within('.milestone') do - expect(page).to have_content "None" + describe 'filtering by due date' do + before do + foo.update(due_date: 1.day.from_now) + bar.update(due_date: 6.days.from_now) end - find('.block.milestone .edit-link').click - sleep 2 # wait for ajax stuff to complete - first('.dropdown-content li').click - sleep 2 - page.within('.milestone') do - expect(page).to have_content 'None' - end + it 'filters by none' do + visit project_issues_path(project, due_date: Issue::NoDueDate.name) - expect(issue.reload.milestone).to be_nil - end - - it 'allows user to de-select milestone', :js do - visit project_issue_path(project, issue) - - page.within('.milestone') do - click_link 'Edit' - click_link milestone.title - - page.within '.value' do - expect(page).to have_content milestone.title + page.within '.issues-holder' do + expect(page).not_to have_content('foo') + expect(page).not_to have_content('bar') + expect(page).to have_content('baz') end + end - click_link 'Edit' - click_link milestone.title + it 'filters by any' do + visit project_issues_path(project, due_date: Issue::AnyDueDate.name) - page.within '.value' do - expect(page).to have_content 'None' + page.within '.issues-holder' do + expect(page).to have_content('foo') + expect(page).to have_content('bar') + expect(page).to have_content('baz') + end + end + + it 'filters by due this week' do + foo.update(due_date: Date.today.beginning_of_week + 2.days) + bar.update(due_date: Date.today.end_of_week) + baz.update(due_date: Date.today - 8.days) + + visit project_issues_path(project, due_date: Issue::DueThisWeek.name) + + page.within '.issues-holder' do + expect(page).to have_content('foo') + expect(page).to have_content('bar') + expect(page).not_to have_content('baz') + end + end + + it 'filters by due this month' do + foo.update(due_date: Date.today.beginning_of_month + 2.days) + bar.update(due_date: Date.today.end_of_month) + baz.update(due_date: Date.today - 50.days) + + visit project_issues_path(project, due_date: Issue::DueThisMonth.name) + + page.within '.issues-holder' do + expect(page).to have_content('foo') + expect(page).to have_content('bar') + expect(page).not_to have_content('baz') + end + end + + it 'filters by overdue' do + foo.update(due_date: Date.today + 2.days) + bar.update(due_date: Date.today + 20.days) + baz.update(due_date: Date.yesterday) + + visit project_issues_path(project, due_date: Issue::Overdue.name) + + page.within '.issues-holder' do + expect(page).not_to have_content('foo') + expect(page).not_to have_content('bar') + expect(page).to have_content('baz') end end end - end - context 'by unauthorized user' do - let(:guest) { create(:user) } - - before do - project.team << [guest, :guest] - issue.milestone = milestone - issue.save - end - - it 'shows milestone text', :js do - sign_out(:user) - sign_in(guest) - - visit project_issue_path(project, issue) - expect(page).to have_content milestone.title - end - end - end - - describe 'new issue' do - let!(:issue) { create(:issue, project: project) } - - context 'by unauthenticated user' do - before do - sign_out(:user) - end - - it 'redirects to signin then back to new issue after signin' do - visit project_issues_path(project) - - page.within '.nav-controls' do - click_link 'New issue' + describe 'sorting by milestone' do + before do + foo.milestone = newer_due_milestone + foo.save + bar.milestone = later_due_milestone + bar.save end - expect(current_path).to eq new_user_session_path + it 'sorts by milestone' do + visit project_issues_path(project, sort: sort_value_milestone) - gitlab_sign_in(create(:user)) - - expect(current_path).to eq new_project_issue_path(project) - end - end - - context 'dropzone upload file', :js do - before do - visit new_project_issue_path(project) + expect(first_issue).to include('foo') + expect(last_issue).to include('baz') + end end - it 'uploads file when dragging into textarea' do - dropzone_file Rails.root.join('spec', 'fixtures', 'banana_sample.gif') + describe 'combine filter and sort' do + let(:user2) { create(:user) } - expect(page.find_field("issue_description").value).to have_content 'banana_sample' - end - - it "doesn't add double newline to end of a single attachment markdown" do - dropzone_file Rails.root.join('spec', 'fixtures', 'banana_sample.gif') - - expect(page.find_field("issue_description").value).not_to match /\n\n$/ - end - - it "cancels a file upload correctly" do - slow_requests do - dropzone_file([Rails.root.join('spec', 'fixtures', 'dk.png')], 0, false) - - click_button 'Cancel' + before do + foo.assignees << user2 + foo.save + bar.assignees << user2 + bar.save end - expect(page).to have_button('Attach a file') - expect(page).not_to have_button('Cancel') - expect(page).not_to have_selector('.uploading-progress-container', visible: true) + it 'sorts with a filter applied' do + visit project_issues_path(project, sort: sort_value_created_date, assignee_id: user2.id) + + expect(first_issue).to include('foo') + expect(last_issue).to include('bar') + expect(page).not_to have_content('baz') + end end end - context 'form filled by URL parameters' do - let(:project) { create(:project, :public, :repository) } + describe 'when I want to reset my incoming email token' do + let(:project1) { create(:project, namespace: user.namespace) } + let!(:issue) { create(:issue, project: project1) } before do - project.repository.create_file( - user, - '.gitlab/issue_templates/bug.md', - 'this is a test "bug" template', - message: 'added issue template', - branch_name: 'master') - - visit new_project_issue_path(project, issuable_template: 'bug') - end - - it 'fills in template' do - expect(find('.js-issuable-selector .dropdown-toggle-text')).to have_content('bug') - end - end - end - - describe 'new issue by email' do - shared_examples 'show the email in the modal' do - let(:issue) { create(:issue, project: project) } - - before do - project.issues << issue stub_incoming_email_setting(enabled: true, address: "p+%{key}@gl.ab") - - visit project_issues_path(project) - click_button('Email a new issue') + project1.team << [user, :master] + visit namespace_project_issues_path(user.namespace, project1) end - it 'click the button to show modal for the new email' do - page.within '#issuable-email-modal' do - email = project.new_issuable_address(user, 'issue') + it 'changes incoming email address token', :js do + find('.issuable-email-modal-btn').click + previous_token = find('input#issuable_email').value + find('.incoming-email-token-reset').click - expect(page).to have_selector("input[value='#{email}']") + wait_for_requests + + expect(page).to have_no_field('issuable_email', with: previous_token) + new_token = project1.new_issuable_address(user.reload, 'issue') + expect(page).to have_field( + 'issuable_email', + with: new_token + ) + end + end + + describe 'update labels from issue#show', :js do + let(:issue) { create(:issue, project: project, author: user, assignees: [user]) } + let!(:label) { create(:label, project: project) } + + before do + visit project_issue_path(project, issue) + end + + it 'will not send ajax request when no data is changed' do + page.within '.labels' do + click_link 'Edit' + + find('.dropdown-menu-close', match: :first).click + + expect(page).not_to have_selector('.block-loading') end end end - context 'with existing issues' do - let!(:issue) { create(:issue, project: project, author: user) } - - it_behaves_like 'show the email in the modal' - end - - context 'without existing issues' do - it_behaves_like 'show the email in the modal' - end - end - - describe 'due date' do - context 'update due on issue#show', :js do + describe 'update assignee from issue#show' do let(:issue) { create(:issue, project: project, author: user, assignees: [user]) } - before do + context 'by authorized user' do + it 'allows user to select unassigned', :js do + visit project_issue_path(project, issue) + + page.within('.assignee') do + expect(page).to have_content "#{user.name}" + + click_link 'Edit' + click_link 'Unassigned' + first('.title').click + expect(page).to have_content 'No assignee' + end + + # wait_for_requests does not work with vue-resource at the moment + sleep 1 + + expect(issue.reload.assignees).to be_empty + end + + it 'allows user to select an assignee', :js do + issue2 = create(:issue, project: project, author: user) + visit project_issue_path(project, issue2) + + page.within('.assignee') do + expect(page).to have_content "No assignee" + end + + page.within '.assignee' do + click_link 'Edit' + end + + page.within '.dropdown-menu-user' do + click_link user.name + end + + page.within('.assignee') do + expect(page).to have_content user.name + end + end + + it 'allows user to unselect themselves', :js do + issue2 = create(:issue, project: project, author: user) + visit project_issue_path(project, issue2) + + page.within '.assignee' do + click_link 'Edit' + click_link user.name + + page.within '.value .author' do + expect(page).to have_content user.name + end + + click_link 'Edit' + click_link user.name + + page.within '.value .assign-yourself' do + expect(page).to have_content "No assignee" + end + end + end + end + + context 'by unauthorized user' do + let(:guest) { create(:user) } + + before do + project.team << [[guest], :guest] + end + + it 'shows assignee text', :js do + sign_out(:user) + sign_in(guest) + + visit project_issue_path(project, issue) + expect(page).to have_content issue.assignees.first.name + end + end + end + + describe 'update milestone from issue#show' do + let!(:issue) { create(:issue, project: project, author: user) } + let!(:milestone) { create(:milestone, project: project) } + + context 'by authorized user' do + it 'allows user to select unassigned', :js do + visit project_issue_path(project, issue) + + page.within('.milestone') do + expect(page).to have_content "None" + end + + find('.block.milestone .edit-link').click + sleep 2 # wait for ajax stuff to complete + first('.dropdown-content li').click + sleep 2 + page.within('.milestone') do + expect(page).to have_content 'None' + end + + expect(issue.reload.milestone).to be_nil + end + + it 'allows user to de-select milestone', :js do + visit project_issue_path(project, issue) + + page.within('.milestone') do + click_link 'Edit' + click_link milestone.title + + page.within '.value' do + expect(page).to have_content milestone.title + end + + click_link 'Edit' + click_link milestone.title + + page.within '.value' do + expect(page).to have_content 'None' + end + end + end + end + + context 'by unauthorized user' do + let(:guest) { create(:user) } + + before do + project.team << [guest, :guest] + issue.milestone = milestone + issue.save + end + + it 'shows milestone text', :js do + sign_out(:user) + sign_in(guest) + + visit project_issue_path(project, issue) + expect(page).to have_content milestone.title + end + end + end + + describe 'new issue' do + let!(:issue) { create(:issue, project: project) } + + context 'by unauthenticated user' do + before do + sign_out(:user) + end + + it 'redirects to signin then back to new issue after signin' do + visit project_issues_path(project) + + page.within '.nav-controls' do + click_link 'New issue' + end + + expect(current_path).to eq new_user_session_path + + gitlab_sign_in(create(:user)) + + expect(current_path).to eq new_project_issue_path(project) + end + end + + context 'dropzone upload file', :js do + before do + visit new_project_issue_path(project) + end + + it 'uploads file when dragging into textarea' do + dropzone_file Rails.root.join('spec', 'fixtures', 'banana_sample.gif') + + expect(page.find_field("issue_description").value).to have_content 'banana_sample' + end + + it "doesn't add double newline to end of a single attachment markdown" do + dropzone_file Rails.root.join('spec', 'fixtures', 'banana_sample.gif') + + expect(page.find_field("issue_description").value).not_to match /\n\n$/ + end + + it "cancels a file upload correctly" do + slow_requests do + dropzone_file([Rails.root.join('spec', 'fixtures', 'dk.png')], 0, false) + + click_button 'Cancel' + end + + expect(page).to have_button('Attach a file') + expect(page).not_to have_button('Cancel') + expect(page).not_to have_selector('.uploading-progress-container', visible: true) + end + end + + context 'form filled by URL parameters' do + let(:project) { create(:project, :public, :repository) } + + before do + project.repository.create_file( + user, + '.gitlab/issue_templates/bug.md', + 'this is a test "bug" template', + message: 'added issue template', + branch_name: 'master') + + visit new_project_issue_path(project, issuable_template: 'bug') + end + + it 'fills in template' do + expect(find('.js-issuable-selector .dropdown-toggle-text')).to have_content('bug') + end + end + end + + describe 'new issue by email' do + shared_examples 'show the email in the modal' do + let(:issue) { create(:issue, project: project) } + + before do + project.issues << issue + stub_incoming_email_setting(enabled: true, address: "p+%{key}@gl.ab") + + visit project_issues_path(project) + click_button('Email a new issue') + end + + it 'click the button to show modal for the new email' do + page.within '#issuable-email-modal' do + email = project.new_issuable_address(user, 'issue') + + expect(page).to have_selector("input[value='#{email}']") + end + end + end + + context 'with existing issues' do + let!(:issue) { create(:issue, project: project, author: user) } + + it_behaves_like 'show the email in the modal' + end + + context 'without existing issues' do + it_behaves_like 'show the email in the modal' + end + end + + describe 'due date' do + context 'update due on issue#show', :js do + let(:issue) { create(:issue, project: project, author: user, assignees: [user]) } + + before do + visit project_issue_path(project, issue) + end + + it 'adds due date to issue' do + date = Date.today.at_beginning_of_month + 2.days + + page.within '.due_date' do + click_link 'Edit' + + page.within '.pika-single' do + click_button date.day + end + + wait_for_requests + + expect(find('.value').text).to have_content date.strftime('%b %-d, %Y') + end + end + + it 'removes due date from issue' do + date = Date.today.at_beginning_of_month + 2.days + + page.within '.due_date' do + click_link 'Edit' + + page.within '.pika-single' do + click_button date.day + end + + wait_for_requests + + expect(page).to have_no_content 'No due date' + + click_link 'remove due date' + expect(page).to have_content 'No due date' + end + end + end + end + + describe 'title issue#show', :js do + it 'updates the title', :js do + issue = create(:issue, author: user, assignees: [user], project: project, title: 'new title') + visit project_issue_path(project, issue) - end - it 'adds due date to issue' do - date = Date.today.at_beginning_of_month + 2.days + expect(page).to have_text("new title") - page.within '.due_date' do - click_link 'Edit' + issue.update(title: "updated title") - page.within '.pika-single' do - click_button date.day - end - - wait_for_requests - - expect(find('.value').text).to have_content date.strftime('%b %-d, %Y') - end - end - - it 'removes due date from issue' do - date = Date.today.at_beginning_of_month + 2.days - - page.within '.due_date' do - click_link 'Edit' - - page.within '.pika-single' do - click_button date.day - end - - wait_for_requests - - expect(page).to have_no_content 'No due date' - - click_link 'remove due date' - expect(page).to have_content 'No due date' - end + wait_for_requests + expect(page).to have_text("updated title") end end - end - describe 'title issue#show', :js do - it 'updates the title', :js do - issue = create(:issue, author: user, assignees: [user], project: project, title: 'new title') + describe 'confidential issue#show', :js do + it 'shows confidential sibebar information as confidential and can be turned off' do + issue = create(:issue, :confidential, project: project) - visit project_issue_path(project, issue) + visit project_issue_path(project, issue) - expect(page).to have_text("new title") + expect(page).to have_css('.issuable-note-warning') + expect(find('.issuable-sidebar-item.confidentiality')).to have_css('.is-active') + expect(find('.issuable-sidebar-item.confidentiality')).not_to have_css('.not-active') - issue.update(title: "updated title") + find('.confidential-edit').click + expect(page).to have_css('.sidebar-item-warning-message') - wait_for_requests - expect(page).to have_text("updated title") - end - end + within('.sidebar-item-warning-message') do + find('.btn-close').click + end - describe 'confidential issue#show', :js do - it 'shows confidential sibebar information as confidential and can be turned off' do - issue = create(:issue, :confidential, project: project) + wait_for_requests - visit project_issue_path(project, issue) + visit project_issue_path(project, issue) - expect(page).to have_css('.issuable-note-warning') - expect(find('.issuable-sidebar-item.confidentiality')).to have_css('.is-active') - expect(find('.issuable-sidebar-item.confidentiality')).not_to have_css('.not-active') - - find('.confidential-edit').click - expect(page).to have_css('.sidebar-item-warning-message') - - within('.sidebar-item-warning-message') do - find('.btn-close').click + expect(page).not_to have_css('.is-active') end - - wait_for_requests - - visit project_issue_path(project, issue) - - expect(page).not_to have_css('.is-active') end end end From 2afb6cbf9d54762c9bd1b7aebb955dc5c29626ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 15 Dec 2017 12:04:58 +0100 Subject: [PATCH 25/50] Use dedicated runners for all the CI jobs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- .gitlab-ci.yml | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index d35fd28c766..2493250f8fb 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -1,5 +1,10 @@ image: "dev.gitlab.org:5005/gitlab/gitlab-build-images:ruby-2.3.5-golang-1.8-git-2.14-chrome-63.0-node-8.x-yarn-1.2-postgresql-9.6" +.dedicated-runner: &dedicated-runner + retry: 1 + tags: + - gitlab-org + .default-cache: &default-cache key: "ruby-235-with-yarn" paths: @@ -42,11 +47,6 @@ stages: - post-cleanup # Predefined scopes -.dedicated-runner: &dedicated-runner - retry: 1 - tags: - - gitlab-org - .tests-metadata-state: &tests-metadata-state <<: *dedicated-runner variables: @@ -82,9 +82,9 @@ stages: .rspec-metadata: &rspec-metadata <<: *dedicated-runner - <<: *pull-cache <<: *except-docs <<: *except-qa + <<: *pull-cache stage: test script: - JOB_NAME=( $CI_JOB_NAME ) @@ -121,9 +121,9 @@ stages: .spinach-metadata: &spinach-metadata <<: *dedicated-runner - <<: *pull-cache <<: *except-docs <<: *except-qa + <<: *pull-cache stage: test script: - JOB_NAME=( $CI_JOB_NAME ) @@ -162,6 +162,7 @@ stages: # Trigger a package build in omnibus-gitlab repository # package-qa: + <<: *dedicated-runner image: ruby:2.4-alpine before_script: [] stage: build @@ -175,6 +176,7 @@ package-qa: # Review docs base .review-docs: &review-docs + <<: *dedicated-runner <<: *except-qa image: ruby:2.4-alpine before_script: @@ -284,9 +286,9 @@ flaky-examples-check: - scripts/detect-new-flaky-examples $NEW_FLAKY_SPECS_REPORT setup-test-env: - <<: *use-pg <<: *dedicated-runner <<: *except-docs + <<: *use-pg stage: prepare cache: <<: *default-cache @@ -375,19 +377,19 @@ spinach-mysql 3 4: *spinach-metadata-mysql SETUP_DB: "false" .rake-exec: &rake-exec - <<: *ruby-static-analysis <<: *dedicated-runner <<: *except-docs <<: *except-qa <<: *pull-cache + <<: *ruby-static-analysis stage: test script: - bundle exec rake $CI_JOB_NAME static-analysis: - <<: *ruby-static-analysis <<: *dedicated-runner <<: *except-docs + <<: *ruby-static-analysis stage: test script: - scripts/static-analysis @@ -456,11 +458,17 @@ db:migrate:reset-mysql: <<: *db-migrate-reset <<: *use-mysql +db:check-schema-pg: + <<: *db-migrate-reset + <<: *use-pg + script: + - source scripts/schema_changed.sh + .migration-paths: &migration-paths <<: *dedicated-runner - <<: *pull-cache <<: *except-docs <<: *except-qa + <<: *pull-cache stage: test variables: SETUP_DB: "false" @@ -530,12 +538,6 @@ db:seed_fu-mysql: <<: *db-seed_fu <<: *use-mysql -db:check-schema-pg: - <<: *db-migrate-reset - <<: *use-pg - script: - - source scripts/schema_changed.sh - # Frontend-related jobs gitlab:assets:compile: <<: *dedicated-runner @@ -561,11 +563,11 @@ gitlab:assets:compile: - webpack-report/ karma: - <<: *use-pg <<: *dedicated-runner <<: *except-docs <<: *except-qa <<: *pull-cache + <<: *use-pg stage: test variables: BABEL_ENV: "coverage" @@ -604,6 +606,7 @@ codequality: paths: [codeclimate.json] qa:internal: + <<: *dedicated-runner <<: *except-docs stage: test variables: @@ -695,9 +698,10 @@ cache gems: - master@gitlab-org/gitlab-ee gitlab_git_test: - <<: *pull-cache + <<: *dedicated-runner <<: *except-docs <<: *except-qa + <<: *pull-cache variables: SETUP_DB: "false" script: From 53381a962d5fef78ae3edbfa9a0d6f9ba3f7187b Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 18 Dec 2017 09:00:08 -0800 Subject: [PATCH 26/50] Add pipeline_creation:run_pipeline_schedule into Sidekiq queue list --- app/workers/all_queues.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index ba31a5aa9c2..268b7028fd9 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -39,6 +39,7 @@ - pipeline_cache:expire_job_cache - pipeline_cache:expire_pipeline_cache - pipeline_creation:create_pipeline +- pipeline_creation:run_pipeline_schedule - pipeline_default:build_coverage - pipeline_default:build_trace_sections - pipeline_default:pipeline_metrics From a794b161cd996f3328c0b9a60d0e3cbdaee2a914 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Mon, 18 Dec 2017 12:02:45 -0500 Subject: [PATCH 27/50] rework the merge_request_version_params to use Object#tap --- app/models/diff_discussion.rb | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb index 758a26d3c70..d67b16584a4 100644 --- a/app/models/diff_discussion.rb +++ b/app/models/diff_discussion.rb @@ -23,9 +23,8 @@ class DiffDiscussion < Discussion def merge_request_version_params return unless for_merge_request? - version_params do |params| + version_params.tap do |params| params[:commit_id] = commit_id if on_merge_request_commit? - params end end @@ -39,11 +38,8 @@ class DiffDiscussion < Discussion private def version_params - params = if active? - {} - else - noteable.version_params_for(position.diff_refs) - end - yield params + return {} if active? + + noteable.version_params_for(position.diff_refs) end end From 52fe6522bdf673fc8de27ea241441a7130f14a97 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Fri, 15 Dec 2017 13:34:26 -0700 Subject: [PATCH 28/50] Remove extra height declarations in CSS; remove block styling from dropdowns --- app/assets/stylesheets/framework/contextual-sidebar.scss | 6 ------ app/assets/stylesheets/framework/dropdowns.scss | 1 - app/assets/stylesheets/framework/sidebar.scss | 6 ------ app/assets/stylesheets/pages/issuable.scss | 3 +-- 4 files changed, 1 insertion(+), 15 deletions(-) diff --git a/app/assets/stylesheets/framework/contextual-sidebar.scss b/app/assets/stylesheets/framework/contextual-sidebar.scss index 8baf7ca23a4..2e417315ed7 100644 --- a/app/assets/stylesheets/framework/contextual-sidebar.scss +++ b/app/assets/stylesheets/framework/contextual-sidebar.scss @@ -9,12 +9,6 @@ padding-left: $contextual-sidebar-width; } - // Override position: absolute - .right-sidebar { - position: fixed; - height: calc(100% - #{$header-height}); - } - .issues-bulk-update.right-sidebar.right-sidebar-expanded .issuable-sidebar-header { padding: 10px 0 15px; } diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index 478269f3fcf..66dddfb35c1 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -214,7 +214,6 @@ .dropdown-menu, .dropdown-menu-nav { @include set-invisible; - display: block; position: absolute; width: auto; top: 100%; diff --git a/app/assets/stylesheets/framework/sidebar.scss b/app/assets/stylesheets/framework/sidebar.scss index 0742c0a2a09..1145a49065f 100644 --- a/app/assets/stylesheets/framework/sidebar.scss +++ b/app/assets/stylesheets/framework/sidebar.scss @@ -89,12 +89,6 @@ .right-sidebar { border-left: 1px solid $border-color; - height: calc(100% - #{$header-height}); - - &.affix { - position: fixed; - top: $header-height; - } } .with-performance-bar .right-sidebar.affix { diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index e19196e0c41..48a787aa737 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -122,7 +122,7 @@ } .right-sidebar { - position: absolute; + position: fixed; top: $header-height; bottom: 0; right: 0; @@ -243,7 +243,6 @@ .issuable-sidebar { width: calc(100% + 100px); - height: 100%; overflow-y: scroll; overflow-x: hidden; -webkit-overflow-scrolling: touch; From 36b3dbf29ab5af30658c7e50028d4040498c82bd Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Fri, 15 Dec 2017 13:46:16 -0700 Subject: [PATCH 29/50] Remove all dropdown animations and set display: none if they're not open --- app/assets/stylesheets/framework/dropdowns.scss | 12 ++---------- app/assets/stylesheets/pages/issuable.scss | 4 ---- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index 66dddfb35c1..22303e6ec14 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -16,20 +16,12 @@ @mixin set-visible { transform: translateY(0); - visibility: visible; - opacity: 1; - transition-duration: 100ms, 150ms, 25ms; - transition-delay: 35ms, 50ms, 25ms; + display: block; } @mixin set-invisible { transform: translateY(-10px); - visibility: hidden; - opacity: 0; - transition-property: opacity, transform, visibility; - transition-duration: 70ms, 250ms, 250ms; - transition-timing-function: linear, $dropdown-animation-timing; - transition-delay: 25ms, 50ms, 0ms; + display: none; } .open { diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index 48a787aa737..4bc7aeb127d 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -499,10 +499,6 @@ .with-performance-bar .right-sidebar { top: $header-height + $performance-bar-height; - - .issuable-sidebar { - height: calc(100% - #{$header-height} - #{$performance-bar-height}); - } } .sidebar-move-issue-confirmation-button { From c5e400405e545dddd68bf79e9a581b093159209d Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Fri, 15 Dec 2017 13:57:28 -0700 Subject: [PATCH 30/50] Fix sidebar height when performance bar enabled --- app/assets/stylesheets/framework/sidebar.scss | 1 + app/assets/stylesheets/pages/issuable.scss | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/app/assets/stylesheets/framework/sidebar.scss b/app/assets/stylesheets/framework/sidebar.scss index 1145a49065f..d61809cb0a4 100644 --- a/app/assets/stylesheets/framework/sidebar.scss +++ b/app/assets/stylesheets/framework/sidebar.scss @@ -89,6 +89,7 @@ .right-sidebar { border-left: 1px solid $border-color; + height: calc(100% - #{$header-height}); } .with-performance-bar .right-sidebar.affix { diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index 4bc7aeb127d..e1637618ab2 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -243,6 +243,7 @@ .issuable-sidebar { width: calc(100% + 100px); + height: 100%; overflow-y: scroll; overflow-x: hidden; -webkit-overflow-scrolling: touch; @@ -499,6 +500,10 @@ .with-performance-bar .right-sidebar { top: $header-height + $performance-bar-height; + + .issuable-sidebar { + height: calc(100% - #{$performance-bar-height}); + } } .sidebar-move-issue-confirmation-button { From 325fdc9f1f9d4e4c9375e024a3d146782fcc9623 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Mon, 18 Dec 2017 11:59:21 -0700 Subject: [PATCH 31/50] Remove block styling from search dropdown --- app/assets/stylesheets/pages/search.scss | 7 ------- 1 file changed, 7 deletions(-) diff --git a/app/assets/stylesheets/pages/search.scss b/app/assets/stylesheets/pages/search.scss index 49c8e546bf2..2381d7eb92b 100644 --- a/app/assets/stylesheets/pages/search.scss +++ b/app/assets/stylesheets/pages/search.scss @@ -108,13 +108,6 @@ input[type="checkbox"]:hover { // Custom dropdown positioning .dropdown-menu { - transition-property: opacity, transform; - transition-duration: 250ms, 250ms; - transition-delay: 0ms, 25ms; - transition-timing-function: $dropdown-animation-timing; - transform: translateY(0); - opacity: 0; - display: block; left: -5px; } From ecba48bdb04ca59b2fec91ac2a2390523ee5e9e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 18 Dec 2017 21:24:07 +0100 Subject: [PATCH 32/50] Inverse the has_multiple_clusters? helper usage --- app/views/projects/clusters/gcp/_form.html.haml | 2 +- app/views/projects/clusters/gcp/_show.html.haml | 2 +- app/views/projects/clusters/user/_form.html.haml | 2 +- app/views/projects/clusters/user/_show.html.haml | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/views/projects/clusters/gcp/_form.html.haml b/app/views/projects/clusters/gcp/_form.html.haml index 21754a561ac..e384b60d8d9 100644 --- a/app/views/projects/clusters/gcp/_form.html.haml +++ b/app/views/projects/clusters/gcp/_form.html.haml @@ -9,7 +9,7 @@ = field.text_field :name, class: 'form-control', placeholder: s_('ClusterIntegration|Cluster name') .form-group = field.label :environment_scope, s_('ClusterIntegration|Environment scope') - = field.text_field :environment_scope, class: 'form-control', readonly: has_multiple_clusters?(@project), placeholder: s_('ClusterIntegration|Environment scope') + = field.text_field :environment_scope, class: 'form-control', readonly: !has_multiple_clusters?(@project), placeholder: s_('ClusterIntegration|Environment scope') = field.fields_for :provider_gcp, @cluster.provider_gcp do |provider_gcp_field| .form-group diff --git a/app/views/projects/clusters/gcp/_show.html.haml b/app/views/projects/clusters/gcp/_show.html.haml index 22a6324ac39..bde85aed341 100644 --- a/app/views/projects/clusters/gcp/_show.html.haml +++ b/app/views/projects/clusters/gcp/_show.html.haml @@ -11,7 +11,7 @@ .form-group = field.label :environment_scope, s_('ClusterIntegration|Environment scope') - = field.text_field :environment_scope, class: 'form-control js-select-on-focus', readonly: has_multiple_clusters?(@project), placeholder: s_('ClusterIntegration|Environment scope') + = field.text_field :environment_scope, class: 'form-control js-select-on-focus', readonly: !has_multiple_clusters?(@project), placeholder: s_('ClusterIntegration|Environment scope') = field.fields_for :platform_kubernetes, @cluster.platform_kubernetes do |platform_kubernetes_field| .form-group diff --git a/app/views/projects/clusters/user/_form.html.haml b/app/views/projects/clusters/user/_form.html.haml index ff575ee2671..babfca0c567 100644 --- a/app/views/projects/clusters/user/_form.html.haml +++ b/app/views/projects/clusters/user/_form.html.haml @@ -5,7 +5,7 @@ = field.text_field :name, class: 'form-control', placeholder: s_('ClusterIntegration|Cluster name') .form-group = field.label :environment_scope, s_('ClusterIntegration|Environment scope') - = field.text_field :environment_scope, class: 'form-control', readonly: has_multiple_clusters?(@project), placeholder: s_('ClusterIntegration|Environment scope') + = field.text_field :environment_scope, class: 'form-control', readonly: !has_multiple_clusters?(@project), placeholder: s_('ClusterIntegration|Environment scope') = field.fields_for :platform_kubernetes, @cluster.platform_kubernetes do |platform_kubernetes_field| .form-group diff --git a/app/views/projects/clusters/user/_show.html.haml b/app/views/projects/clusters/user/_show.html.haml index b07a98e9ef4..89595bca007 100644 --- a/app/views/projects/clusters/user/_show.html.haml +++ b/app/views/projects/clusters/user/_show.html.haml @@ -6,7 +6,7 @@ .form-group = field.label :environment_scope, s_('ClusterIntegration|Environment scope') - = field.text_field :environment_scope, class: 'form-control js-select-on-focus', readonly: has_multiple_clusters?(@project), placeholder: s_('ClusterIntegration|Environment scope') + = field.text_field :environment_scope, class: 'form-control js-select-on-focus', readonly: !has_multiple_clusters?(@project), placeholder: s_('ClusterIntegration|Environment scope') = field.fields_for :platform_kubernetes, @cluster.platform_kubernetes do |platform_kubernetes_field| .form-group From 9611a410be296a2ef2e8ae7df69830dce293199b Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 18 Dec 2017 14:11:00 -0800 Subject: [PATCH 33/50] Fix missing WHERE clause in 20171106135924_issues_milestone_id_foreign_key migration If milestone_id is NULL, we shouldn't need to update it to be NULL again. This was causing us to touch almost all rows in the issues table for no good reason. Closes https://gitlab.com/gitlab-com/infrastructure/issues/3416 --- db/migrate/20171106135924_issues_milestone_id_foreign_key.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/db/migrate/20171106135924_issues_milestone_id_foreign_key.rb b/db/migrate/20171106135924_issues_milestone_id_foreign_key.rb index e6a780d0964..bfb3dcae511 100644 --- a/db/migrate/20171106135924_issues_milestone_id_foreign_key.rb +++ b/db/migrate/20171106135924_issues_milestone_id_foreign_key.rb @@ -16,6 +16,7 @@ class IssuesMilestoneIdForeignKey < ActiveRecord::Migration def self.with_orphaned_milestones where('NOT EXISTS (SELECT true FROM milestones WHERE milestones.id = issues.milestone_id)') + .where('milestone_id IS NOT NULL') end end From b03789395c59ce94c8bdb4e0f4806c8cd7705f46 Mon Sep 17 00:00:00 2001 From: Mario de la Ossa Date: Mon, 18 Dec 2017 22:55:51 -0600 Subject: [PATCH 34/50] Do not generate links for private NPM modules in blob view --- app/models/blob_viewer/dependency_manager.rb | 13 ++++-- app/models/blob_viewer/package_json.rb | 12 +++++ .../viewers/_dependency_manager.html.haml | 2 +- .../unreleased/36020-private-npm-modules.yml | 5 ++ spec/models/blob_viewer/package_json_spec.rb | 46 +++++++++++++++++++ 5 files changed, 74 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/36020-private-npm-modules.yml diff --git a/app/models/blob_viewer/dependency_manager.rb b/app/models/blob_viewer/dependency_manager.rb index a8d9be945dc..cc4950240af 100644 --- a/app/models/blob_viewer/dependency_manager.rb +++ b/app/models/blob_viewer/dependency_manager.rb @@ -27,10 +27,17 @@ module BlobViewer private - def package_name_from_json(key) - prepare! + def json_data + @json_data ||= begin + prepare! + JSON.parse(blob.data) + rescue + {} + end + end - JSON.parse(blob.data)[key] rescue nil + def package_name_from_json(key) + json_data[key] end def package_name_from_method_call(name) diff --git a/app/models/blob_viewer/package_json.rb b/app/models/blob_viewer/package_json.rb index 09221efb56c..6ce61e30d3d 100644 --- a/app/models/blob_viewer/package_json.rb +++ b/app/models/blob_viewer/package_json.rb @@ -16,8 +16,20 @@ module BlobViewer @package_name ||= package_name_from_json('name') end + def package_type + private? ? 'private package' : super + end + def package_url + return nil if private? + "https://www.npmjs.com/package/#{package_name}" end + + private + + def private? + !!json_data['private'] + end end end diff --git a/app/views/projects/blob/viewers/_dependency_manager.html.haml b/app/views/projects/blob/viewers/_dependency_manager.html.haml index a0f0215a5ff..87aa7c1dbf8 100644 --- a/app/views/projects/blob/viewers/_dependency_manager.html.haml +++ b/app/views/projects/blob/viewers/_dependency_manager.html.haml @@ -6,6 +6,6 @@ - if viewer.package_name and defines a #{viewer.package_type} named %strong< - = link_to viewer.package_name, viewer.package_url, target: '_blank', rel: 'noopener noreferrer' + = link_to_if viewer.package_url.present?, viewer.package_name, viewer.package_url, target: '_blank', rel: 'noopener noreferrer' = link_to 'Learn more', viewer.manager_url, target: '_blank', rel: 'noopener noreferrer' diff --git a/changelogs/unreleased/36020-private-npm-modules.yml b/changelogs/unreleased/36020-private-npm-modules.yml new file mode 100644 index 00000000000..a0122e2b360 --- /dev/null +++ b/changelogs/unreleased/36020-private-npm-modules.yml @@ -0,0 +1,5 @@ +--- +title: Do not generate links for private NPM modules in blob view +merge_request: 16002 +author: Mario de la Ossa +type: added diff --git a/spec/models/blob_viewer/package_json_spec.rb b/spec/models/blob_viewer/package_json_spec.rb index 0f8330e91c1..339d4e9e644 100644 --- a/spec/models/blob_viewer/package_json_spec.rb +++ b/spec/models/blob_viewer/package_json_spec.rb @@ -22,4 +22,50 @@ describe BlobViewer::PackageJson do expect(subject.package_name).to eq('module-name') end end + + describe '#package_url' do + it 'returns the package URL' do + expect(subject).to receive(:prepare!) + + expect(subject.package_url).to eq("https://www.npmjs.com/package/#{subject.package_name}") + end + end + + describe '#package_type' do + it 'returns "package"' do + expect(subject).to receive(:prepare!) + + expect(subject.package_type).to eq('package') + end + end + + context 'when package.json has "private": true' do + let(:data) do + <<-SPEC.strip_heredoc + { + "name": "module-name", + "version": "10.3.1", + "private": true + } + SPEC + end + let(:blob) { fake_blob(path: 'package.json', data: data) } + subject { described_class.new(blob) } + + describe '#package_url' do + it 'returns nil' do + expect(subject).to receive(:prepare!) + + expect(subject.package_url).to be_nil + end + end + + describe '#package_type' do + it 'returns "private package"' do + expect(subject).to receive(:prepare!) + + expect(subject.package_type).to eq('private package') + end + end + end end From d02059ddf3fef105a8e835024b589d9eac2140f0 Mon Sep 17 00:00:00 2001 From: Mario de la Ossa Date: Mon, 18 Dec 2017 23:14:08 -0600 Subject: [PATCH 35/50] BlobViewer::PackageJson - if private link to homepage --- app/models/blob_viewer/package_json.rb | 12 +++++++++--- changelogs/unreleased/36020-private-npm-modules.yml | 2 +- spec/models/blob_viewer/package_json_spec.rb | 7 ++++--- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/app/models/blob_viewer/package_json.rb b/app/models/blob_viewer/package_json.rb index 6ce61e30d3d..46cd2f04f4d 100644 --- a/app/models/blob_viewer/package_json.rb +++ b/app/models/blob_viewer/package_json.rb @@ -21,9 +21,7 @@ module BlobViewer end def package_url - return nil if private? - - "https://www.npmjs.com/package/#{package_name}" + private? ? homepage : npm_url end private @@ -31,5 +29,13 @@ module BlobViewer def private? !!json_data['private'] end + + def homepage + json_data['homepage'] + end + + def npm_url + "https://www.npmjs.com/package/#{package_name}" + end end end diff --git a/changelogs/unreleased/36020-private-npm-modules.yml b/changelogs/unreleased/36020-private-npm-modules.yml index a0122e2b360..5c2585a602e 100644 --- a/changelogs/unreleased/36020-private-npm-modules.yml +++ b/changelogs/unreleased/36020-private-npm-modules.yml @@ -1,5 +1,5 @@ --- -title: Do not generate links for private NPM modules in blob view +title: Do not generate NPM links for private NPM modules in blob view merge_request: 16002 author: Mario de la Ossa type: added diff --git a/spec/models/blob_viewer/package_json_spec.rb b/spec/models/blob_viewer/package_json_spec.rb index 339d4e9e644..5ed2f4400bc 100644 --- a/spec/models/blob_viewer/package_json_spec.rb +++ b/spec/models/blob_viewer/package_json_spec.rb @@ -45,7 +45,8 @@ describe BlobViewer::PackageJson do { "name": "module-name", "version": "10.3.1", - "private": true + "private": true, + "homepage": "myawesomepackage.com" } SPEC end @@ -53,10 +54,10 @@ describe BlobViewer::PackageJson do subject { described_class.new(blob) } describe '#package_url' do - it 'returns nil' do + it 'returns homepage if any' do expect(subject).to receive(:prepare!) - expect(subject.package_url).to be_nil + expect(subject.package_url).to eq('myawesomepackage.com') end end From c6edae38870a4228e3b964d647b9ef588df11f27 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 5 Dec 2017 14:15:30 +0100 Subject: [PATCH 36/50] Load commit in batches for pipelines#index Uses `list_commits_by_oid` on the CommitService, to request the needed commits for pipelines. These commits are needed to display the user that created the commit and the commit title. This includes fixes for tests failing that depended on the commit being `nil`. However, now these are batch loaded, this doesn't happen anymore and the commits are an instance of BatchLoader. --- Gemfile | 2 +- Gemfile.lock | 4 +- .../projects/pipelines_controller.rb | 2 + app/models/ci/pipeline.rb | 13 ++--- app/models/commit.rb | 20 ++++++-- app/models/repository.rb | 12 +++++ app/views/projects/pipelines/_info.html.haml | 50 +++++++++---------- app/workers/expire_pipeline_cache_worker.rb | 2 +- lib/gitlab/git/commit.rb | 13 +++++ lib/gitlab/gitaly_client/commit_service.rb | 9 ++++ .../projects/pipelines_controller_spec.rb | 13 ++--- spec/models/commit_spec.rb | 39 +++++++++++++++ spec/models/repository_spec.rb | 48 ++++++++++++++++++ spec/serializers/pipeline_serializer_spec.rb | 6 +-- 14 files changed, 184 insertions(+), 49 deletions(-) diff --git a/Gemfile b/Gemfile index 6b1c6e16851..b6ffaf80f24 100644 --- a/Gemfile +++ b/Gemfile @@ -263,7 +263,7 @@ gem 'gettext_i18n_rails', '~> 1.8.0' gem 'gettext_i18n_rails_js', '~> 1.2.0' gem 'gettext', '~> 3.2.2', require: false, group: :development -gem 'batch-loader' +gem 'batch-loader', '~> 1.2.1' # Perf bar gem 'peek', '~> 1.0.1' diff --git a/Gemfile.lock b/Gemfile.lock index 11040fab805..a6e3c9e27cc 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -78,7 +78,7 @@ GEM thread_safe (~> 0.3, >= 0.3.1) babosa (1.0.2) base32 (0.3.2) - batch-loader (1.1.1) + batch-loader (1.2.1) bcrypt (3.1.11) bcrypt_pbkdf (1.0.0) benchmark-ips (2.3.0) @@ -988,7 +988,7 @@ DEPENDENCIES awesome_print (~> 1.2.0) babosa (~> 1.0.2) base32 (~> 0.3.0) - batch-loader + batch-loader (~> 1.2.1) bcrypt_pbkdf (~> 1.0) benchmark-ips (~> 2.3.0) better_errors (~> 2.1.0) diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb index 7ad7b3003af..e146d0d3cd5 100644 --- a/app/controllers/projects/pipelines_controller.rb +++ b/app/controllers/projects/pipelines_controller.rb @@ -29,6 +29,8 @@ class Projects::PipelinesController < Projects::ApplicationController @pipelines_count = PipelinesFinder .new(project).execute.count + @pipelines.map(&:commit) # List commits for batch loading + respond_to do |format| format.html format.json do diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 28f154581a9..d4690da3be6 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -287,8 +287,12 @@ module Ci Ci::Pipeline.truncate_sha(sha) end + # NOTE: This is loaded lazily and will never be nil, even if the commit + # cannot be found. + # + # Use constructs like: `pipeline.commit.present?` def commit - @commit ||= project.commit_by(oid: sha) + @commit ||= Commit.lazy(project, sha) end def branch? @@ -338,12 +342,9 @@ module Ci end def latest? - return false unless ref + return false unless ref && commit.present? - commit = project.commit(ref) - return false unless commit - - commit.sha == sha + project.commit(ref) == commit end def retried diff --git a/app/models/commit.rb b/app/models/commit.rb index 13c31111134..2be07ca7d3c 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -86,6 +86,20 @@ class Commit def valid_hash?(key) !!(/\A#{COMMIT_SHA_PATTERN}\z/ =~ key) end + + def lazy(project, oid) + BatchLoader.for({ project: project, oid: oid }).batch do |items, loader| + items_by_project = items.group_by { |i| i[:project] } + + items_by_project.each do |project, commit_ids| + oids = commit_ids.map { |i| i[:oid] } + + project.repository.commits_by(oids: oids).each do |commit| + loader.call({ project: commit.project, oid: commit.id }, commit) if commit + end + end + end + end end attr_accessor :raw @@ -103,7 +117,7 @@ class Commit end def ==(other) - (self.class === other) && (raw == other.raw) + other.is_a?(self.class) && raw == other.raw end def self.reference_prefix @@ -224,8 +238,8 @@ class Commit notes.includes(:author) end - def method_missing(m, *args, &block) - @raw.__send__(m, *args, &block) # rubocop:disable GitlabSecurity/PublicSend + def method_missing(method, *args, &block) + @raw.__send__(method, *args, &block) # rubocop:disable GitlabSecurity/PublicSend end def respond_to_missing?(method, include_private = false) diff --git a/app/models/repository.rb b/app/models/repository.rb index 552a354d1ce..fc7081a9f34 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -118,6 +118,18 @@ class Repository @commit_cache[oid] = find_commit(oid) end + def commits_by(oids:) + return [] unless oids.present? + + commits = Gitlab::Git::Commit.batch_by_oid(raw_repository, oids) + + if commits.present? + Commit.decorate(commits, @project) + else + [] + end + end + def commits(ref, path: nil, limit: nil, offset: nil, skip_merges: false, after: nil, before: nil) options = { repo: raw_repository, diff --git a/app/views/projects/pipelines/_info.html.haml b/app/views/projects/pipelines/_info.html.haml index 01ea9356af5..85946aec1f2 100644 --- a/app/views/projects/pipelines/_info.html.haml +++ b/app/views/projects/pipelines/_info.html.haml @@ -1,6 +1,6 @@ #js-pipeline-header-vue.pipeline-header-container -- if @commit +- if @commit.present? .commit-box %h3.commit-title = markdown(@commit.title, pipeline: :single_line) @@ -8,28 +8,28 @@ %pre.commit-description = preserve(markdown(@commit.description, pipeline: :single_line)) -.info-well - - if @commit.status - .well-segment.pipeline-info - .icon-container - = icon('clock-o') - = pluralize @pipeline.total_size, "job" - - if @pipeline.ref - from - = link_to @pipeline.ref, project_ref_path(@project, @pipeline.ref), class: "ref-name" - - if @pipeline.duration - in - = time_interval_in_words(@pipeline.duration) - - if @pipeline.queued_duration - = "(queued for #{time_interval_in_words(@pipeline.queued_duration)})" + .info-well + - if @commit.status + .well-segment.pipeline-info + .icon-container + = icon('clock-o') + = pluralize @pipeline.total_size, "job" + - if @pipeline.ref + from + = link_to @pipeline.ref, project_ref_path(@project, @pipeline.ref), class: "ref-name" + - if @pipeline.duration + in + = time_interval_in_words(@pipeline.duration) + - if @pipeline.queued_duration + = "(queued for #{time_interval_in_words(@pipeline.queued_duration)})" - .well-segment.branch-info - .icon-container.commit-icon - = custom_icon("icon_commit") - = link_to @commit.short_id, project_commit_path(@project, @pipeline.sha), class: "commit-sha js-details-short" - = link_to("#", class: "js-details-expand hidden-xs hidden-sm") do - %span.text-expander - \... - %span.js-details-content.hide - = link_to @pipeline.sha, project_commit_path(@project, @pipeline.sha), class: "commit-sha commit-hash-full" - = clipboard_button(text: @pipeline.sha, title: "Copy commit SHA to clipboard") + .well-segment.branch-info + .icon-container.commit-icon + = custom_icon("icon_commit") + = link_to @commit.short_id, project_commit_path(@project, @pipeline.sha), class: "commit-sha js-details-short" + = link_to("#", class: "js-details-expand hidden-xs hidden-sm") do + %span.text-expander + \... + %span.js-details-content.hide + = link_to @pipeline.sha, project_commit_path(@project, @pipeline.sha), class: "commit-sha commit-hash-full" + = clipboard_button(text: @pipeline.sha, title: "Copy commit SHA to clipboard") diff --git a/app/workers/expire_pipeline_cache_worker.rb b/app/workers/expire_pipeline_cache_worker.rb index 3e34de22c19..db73d37868a 100644 --- a/app/workers/expire_pipeline_cache_worker.rb +++ b/app/workers/expire_pipeline_cache_worker.rb @@ -13,7 +13,7 @@ class ExpirePipelineCacheWorker store.touch(project_pipelines_path(project)) store.touch(project_pipeline_path(project, pipeline)) - store.touch(commit_pipelines_path(project, pipeline.commit)) if pipeline.commit + store.touch(commit_pipelines_path(project, pipeline.commit)) unless pipeline.commit.nil? store.touch(new_merge_request_pipelines_path(project)) each_pipelines_merge_request_path(project, pipeline) do |path| store.touch(path) diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb index e90b158fb34..145721dea76 100644 --- a/lib/gitlab/git/commit.rb +++ b/lib/gitlab/git/commit.rb @@ -228,6 +228,19 @@ module Gitlab end end end + + # Only to be used when the object ids will not necessarily have a + # relation to each other. The last 10 commits for a branch for example, + # should go through .where + def batch_by_oid(repo, oids) + repo.gitaly_migrate(:list_commits_by_oid) do |is_enabled| + if is_enabled + repo.gitaly_commit_client.list_commits_by_oid(oids) + else + oids.map { |oid| find(repo, oid) }.compact + end + end + end end def initialize(repository, raw_commit, head = nil) diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index 7985f5b5457..fb3e27770b4 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/lib/gitlab/gitaly_client/commit_service.rb @@ -169,6 +169,15 @@ module Gitlab consume_commits_response(response) end + def list_commits_by_oid(oids) + request = Gitaly::ListCommitsByOidRequest.new(repository: @gitaly_repo, oid: oids) + + response = GitalyClient.call(@repository.storage, :commit_service, :list_commits_by_oid, request, timeout: GitalyClient.medium_timeout) + consume_commits_response(response) + rescue GRPC::Unknown # If no repository is found, happens mainly during testing + [] + end + def commits_by_message(query, revision: '', path: '', limit: 1000, offset: 0) request = Gitaly::CommitsByMessageRequest.new( repository: @gitaly_repo, diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb index 1604a2da485..35ac999cc65 100644 --- a/spec/controllers/projects/pipelines_controller_spec.rb +++ b/spec/controllers/projects/pipelines_controller_spec.rb @@ -17,13 +17,10 @@ describe Projects::PipelinesController do describe 'GET index.json' do before do - branch_head = project.commit - parent = branch_head.parent - - create(:ci_empty_pipeline, status: 'pending', project: project, sha: branch_head.id) - create(:ci_empty_pipeline, status: 'running', project: project, sha: branch_head.id) - create(:ci_empty_pipeline, status: 'created', project: project, sha: parent.id) - create(:ci_empty_pipeline, status: 'success', project: project, sha: parent.id) + %w(pending running created success).each_with_index do |status, index| + sha = project.commit("HEAD~#{index}") + create(:ci_empty_pipeline, status: status, project: project, sha: sha) + end end subject do @@ -46,7 +43,7 @@ describe Projects::PipelinesController do context 'when performing gitaly calls', :request_store do it 'limits the Gitaly requests' do - expect { subject }.to change { Gitlab::GitalyClient.get_request_count }.by(8) + expect { subject }.to change { Gitlab::GitalyClient.get_request_count }.by(3) end end end diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index d18a5c9dfa6..cd955a5eb69 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -13,6 +13,45 @@ describe Commit do it { is_expected.to include_module(StaticModel) } end + describe '.lazy' do + set(:project) { create(:project, :repository) } + + context 'when the commits are found' do + let(:oids) do + %w( + 498214de67004b1da3d820901307bed2a68a8ef6 + c642fe9b8b9f28f9225d7ea953fe14e74748d53b + 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 + 048721d90c449b244b7b4c53a9186b04330174ec + 281d3a76f31c812dbf48abce82ccf6860adedd81 + ) + end + + subject { oids.map { |oid| described_class.lazy(project, oid) } } + + it 'batches requests for commits' do + expect(project.repository).to receive(:commits_by).once.and_call_original + + subject.first.title + subject.last.title + end + + it 'maintains ordering' do + subject.each_with_index do |commit, i| + expect(commit.id).to eq(oids[i]) + end + end + end + + context 'when not found' do + it 'returns nil as commit' do + commit = described_class.lazy(project, 'deadbeef').__sync + + expect(commit).to be_nil + end + end + end + describe '#author' do it 'looks up the author in a case-insensitive way' do user = create(:user, email: commit.author_email.upcase) diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 799d99c0369..8a3531c1898 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -239,6 +239,54 @@ describe Repository do end end + describe '#commits_by' do + set(:project) { create(:project, :repository) } + + shared_examples 'batch commits fetching' do + let(:oids) { TestEnv::BRANCH_SHA.values } + + subject { project.repository.commits_by(oids: oids) } + + it 'finds each commit' do + expect(subject).not_to include(nil) + expect(subject.size).to eq(oids.size) + end + + it 'returns only Commit instances' do + expect(subject).to all( be_a(Commit) ) + end + + context 'when some commits are not found ' do + let(:oids) do + ['deadbeef'] + TestEnv::BRANCH_SHA.values.first(10) + end + + it 'returns only found commits' do + expect(subject).not_to include(nil) + expect(subject.size).to eq(10) + end + end + + context 'when no oids are passed' do + let(:oids) { [] } + + it 'does not call #batch_by_oid' do + expect(Gitlab::Git::Commit).not_to receive(:batch_by_oid) + + subject + end + end + end + + context 'when Gitaly list_commits_by_oid is enabled' do + it_behaves_like 'batch commits fetching' + end + + context 'when Gitaly list_commits_by_oid is enabled', :disable_gitaly do + it_behaves_like 'batch commits fetching' + end + end + describe '#find_commits_by_message' do shared_examples 'finding commits by message' do it 'returns commits with messages containing a given string' do diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb index 88d347322a6..c38795ad1a1 100644 --- a/spec/serializers/pipeline_serializer_spec.rb +++ b/spec/serializers/pipeline_serializer_spec.rb @@ -1,6 +1,7 @@ require 'spec_helper' describe PipelineSerializer do + set(:project) { create(:project, :repository) } set(:user) { create(:user) } let(:serializer) do @@ -16,7 +17,7 @@ describe PipelineSerializer do end context 'when a single object is being serialized' do - let(:resource) { create(:ci_empty_pipeline) } + let(:resource) { create(:ci_empty_pipeline, project: project) } it 'serializers the pipeline object' do expect(subject[:id]).to eq resource.id @@ -24,7 +25,7 @@ describe PipelineSerializer do end context 'when multiple objects are being serialized' do - let(:resource) { create_list(:ci_pipeline, 2) } + let(:resource) { create_list(:ci_pipeline, 2, project: project) } it 'serializers the array of pipelines' do expect(subject).not_to be_empty @@ -100,7 +101,6 @@ describe PipelineSerializer do context 'number of queries' do let(:resource) { Ci::Pipeline.all } - let(:project) { create(:project) } before do # Since RequestStore.active? is true we have to allow the From 873bc3a6854689549cf426f726499619205a2798 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 18 Dec 2017 21:19:57 +0800 Subject: [PATCH 37/50] Preserve gem path so that we use the same gems Without this patch, I would end up with: ``` An error occurred in a `before(:suite)` hook. Failure/Error: raise "could not connect to gitaly at #{socket.inspect} after #{sleep_time} seconds" RuntimeError: could not connect to gitaly at "tmp/tests/gitaly/gitaly.socket" after 10 seconds ``` Digging into it, it's because `scripts/gitaly-test-spawn` could not spawn the process, because it cannot find the installed gems. I personally installed all my gems under $HOME, namely with: * `gem install rake --user-install` or: * `bundle install --path ~/.gem` The gems would be installed to `~/.gem/ruby/2.4.0/gems`, where the version is Ruby ABI version. Now we're changing $HOME, making RubyGems think that the gems would be installed to `tmp/tests/ruby/2.4.0/gems` which is apparently not the case. In order to workaround this, we could preserve $GEM_PATH populated by RubyGems, ignoring the default path based on $HOME. --- scripts/gitaly-test-spawn | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/gitaly-test-spawn b/scripts/gitaly-test-spawn index 8e05eca8d7e..ecb68c6acc6 100755 --- a/scripts/gitaly-test-spawn +++ b/scripts/gitaly-test-spawn @@ -1,7 +1,8 @@ #!/usr/bin/env ruby gitaly_dir = 'tmp/tests/gitaly' -env = { 'HOME' => File.expand_path('tmp/tests') } +env = { 'HOME' => File.expand_path('tmp/tests'), + 'GEM_PATH' => Gem.path.join(':') } args = %W[#{gitaly_dir}/gitaly #{gitaly_dir}/config.toml] # Print the PID of the spawned process From f3f9dedcf70c1bf247945253f3cd9026ab87d421 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 19 Dec 2017 10:46:13 +0000 Subject: [PATCH 38/50] Remove transitionend event from GL dropdown --- app/assets/javascripts/gl_dropdown.js | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/app/assets/javascripts/gl_dropdown.js b/app/assets/javascripts/gl_dropdown.js index cf4a70e321e..64f258aed64 100644 --- a/app/assets/javascripts/gl_dropdown.js +++ b/app/assets/javascripts/gl_dropdown.js @@ -300,7 +300,7 @@ GitLabDropdown = (function() { return function(data) { _this.fullData = data; _this.parseData(_this.fullData); - _this.focusTextInput(true); + _this.focusTextInput(); if (_this.options.filterable && _this.filter && _this.filter.input && _this.filter.input.val() && _this.filter.input.val().trim() !== '') { return _this.filter.input.trigger('input'); } @@ -790,24 +790,16 @@ GitLabDropdown = (function() { return [selectedObject, isMarking]; }; - GitLabDropdown.prototype.focusTextInput = function(triggerFocus = false) { + GitLabDropdown.prototype.focusTextInput = function() { if (this.options.filterable) { - this.dropdown.one('transitionend', () => { - const initialScrollTop = $(window).scrollTop(); + const initialScrollTop = $(window).scrollTop(); - if (this.dropdown.is('.open')) { - this.filterInput.focus(); - } + if (this.dropdown.is('.open')) { + this.filterInput.focus(); + } - if ($(window).scrollTop() < initialScrollTop) { - $(window).scrollTop(initialScrollTop); - } - }); - - if (triggerFocus) { - // This triggers after a ajax request - // in case of slow requests, the dropdown transition could already be finished - this.dropdown.trigger('transitionend'); + if ($(window).scrollTop() < initialScrollTop) { + $(window).scrollTop(initialScrollTop); } } }; From 08949a189863172a8c9d69680a9c5c2f79ff9202 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Tue, 19 Dec 2017 11:13:02 +0000 Subject: [PATCH 39/50] Update axios.md --- doc/development/fe_guide/axios.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/development/fe_guide/axios.md b/doc/development/fe_guide/axios.md index 962fe3dcec9..1daa6758171 100644 --- a/doc/development/fe_guide/axios.md +++ b/doc/development/fe_guide/axios.md @@ -11,7 +11,7 @@ This exported module should be used instead of directly using `axios` to ensure ## Usage ```javascript - import axios from '~/lib/utils/axios_utils'; + import axios from './lib/utils/axios_utils'; axios.get(url) .then((response) => { From 73f615a5c6be79a6d464f25575d8ba3f1c98a0a2 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 19 Dec 2017 19:43:50 +0800 Subject: [PATCH 40/50] Make sure two except won't overwrite each other This is a pretty boring solution, but I can't think of a good idea right now and this might be good enough for now... --- .gitlab-ci.yml | 44 ++++++++++++++++++-------------------------- 1 file changed, 18 insertions(+), 26 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 2493250f8fb..c26e7f0aeba 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -80,10 +80,14 @@ stages: except: - /(^qa[\/-].*|.*-qa$)/ +.except-docs-and-qa: &except-docs-and-qa + except: + - /(^docs[\/-].*|.*-docs$)/ + - /(^qa[\/-].*|.*-qa$)/ + .rspec-metadata: &rspec-metadata <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache stage: test script: @@ -121,8 +125,7 @@ stages: .spinach-metadata: &spinach-metadata <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache stage: test script: @@ -222,8 +225,7 @@ review-docs-cleanup: # Retrieve knapsack and rspec_flaky reports retrieve-tests-metadata: <<: *tests-metadata-state - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa stage: prepare cache: key: tests_metadata @@ -378,8 +380,7 @@ spinach-mysql 3 4: *spinach-metadata-mysql .rake-exec: &rake-exec <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache <<: *ruby-static-analysis stage: test @@ -443,8 +444,7 @@ ee_compat_check: # DB migration, rollback, and seed jobs .db-migrate-reset: &db-migrate-reset <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache stage: test script: @@ -466,8 +466,7 @@ db:check-schema-pg: .migration-paths: &migration-paths <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache stage: test variables: @@ -494,8 +493,7 @@ migration:path-mysql: .db-rollback: &db-rollback <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache stage: test script: @@ -512,8 +510,7 @@ db:rollback-mysql: .db-seed_fu: &db-seed_fu <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache stage: test variables: @@ -541,8 +538,7 @@ db:seed_fu-mysql: # Frontend-related jobs gitlab:assets:compile: <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache stage: test dependencies: [] @@ -564,8 +560,7 @@ gitlab:assets:compile: karma: <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache <<: *use-pg stage: test @@ -619,8 +614,7 @@ qa:internal: coverage: <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache stage: post-test services: [] @@ -639,8 +633,7 @@ coverage: lint:javascript:report: <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache stage: post-test dependencies: @@ -699,8 +692,7 @@ cache gems: gitlab_git_test: <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache variables: SETUP_DB: "false" From d86cd7c3d9a4613540e2fd81ea3f31795be7917c Mon Sep 17 00:00:00 2001 From: Richard Clamp Date: Tue, 19 Dec 2017 14:53:27 +0000 Subject: [PATCH 41/50] Tidy up the documentation of Gitlab HA/Gitlab Application --- .../high_availability/gitlab.md | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/doc/administration/high_availability/gitlab.md b/doc/administration/high_availability/gitlab.md index 42666357faf..b85a166089d 100644 --- a/doc/administration/high_availability/gitlab.md +++ b/doc/administration/high_availability/gitlab.md @@ -1,6 +1,6 @@ # Configuring GitLab for HA -Assuming you have already configured a database, Redis, and NFS, you can +Assuming you have already configured a [database](database.md), [Redis](redis.md), and [NFS](nfs.md), you can configure the GitLab application server(s) now. Complete the steps below for each GitLab application server in your environment. @@ -48,34 +48,33 @@ for each GitLab application server in your environment. data locations. See [NFS documentation](nfs.md) for `/etc/gitlab/gitlab.rb` configuration values for various scenarios. The example below assumes you've added NFS mounts in the default data locations. - + ```ruby external_url 'https://gitlab.example.com' # Prevent GitLab from starting if NFS data mounts are not available high_availability['mountpoint'] = '/var/opt/gitlab/git-data' - + # Disable components that will not be on the GitLab application server - postgresql['enable'] = false - redis['enable'] = false - + roles ['application_role'] + # PostgreSQL connection details gitlab_rails['db_adapter'] = 'postgresql' gitlab_rails['db_encoding'] = 'unicode' gitlab_rails['db_host'] = '10.1.0.5' # IP/hostname of database server gitlab_rails['db_password'] = 'DB password' - + # Redis connection details gitlab_rails['redis_port'] = '6379' gitlab_rails['redis_host'] = '10.1.0.6' # IP/hostname of Redis server gitlab_rails['redis_password'] = 'Redis Password' ``` - - > **Note:** To maintain uniformity of links across HA clusters, the `external_url` - on the first application server as well as the additional application - servers should point to the external url that users will use to access GitLab. + + > **Note:** To maintain uniformity of links across HA clusters, the `external_url` + on the first application server as well as the additional application + servers should point to the external url that users will use to access GitLab. In a typical HA setup, this will be the url of the load balancer which will - route traffic to all GitLab application servers in the HA cluster. + route traffic to all GitLab application servers in the HA cluster. 1. Run `sudo gitlab-ctl reconfigure` to compile the configuration. From 8b13bdbe4d5805727c2c165f8cf0c7593c4af92d Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Tue, 19 Dec 2017 13:52:57 +0200 Subject: [PATCH 42/50] [CE->EE] Fix spec/lib/gitlab/git/gitlab_projects_spec.rb --- spec/lib/gitlab/git/gitlab_projects_spec.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spec/lib/gitlab/git/gitlab_projects_spec.rb b/spec/lib/gitlab/git/gitlab_projects_spec.rb index 18906955df6..24da9589458 100644 --- a/spec/lib/gitlab/git/gitlab_projects_spec.rb +++ b/spec/lib/gitlab/git/gitlab_projects_spec.rb @@ -41,7 +41,8 @@ describe Gitlab::Git::GitlabProjects do end it "fails if the source path doesn't exist" do - expect(logger).to receive(:error).with("mv-project failed: source path <#{tmp_repos_path}/bad-src.git> does not exist.") + expected_source_path = File.join(tmp_repos_path, 'bad-src.git') + expect(logger).to receive(:error).with("mv-project failed: source path <#{expected_source_path}> does not exist.") result = build_gitlab_projects(tmp_repos_path, 'bad-src.git').mv_project('repo.git') expect(result).to be_falsy @@ -50,7 +51,8 @@ describe Gitlab::Git::GitlabProjects do it 'fails if the destination path already exists' do FileUtils.mkdir_p(File.join(tmp_repos_path, 'already-exists.git')) - message = "mv-project failed: destination path <#{tmp_repos_path}/already-exists.git> already exists." + expected_distination_path = File.join(tmp_repos_path, 'already-exists.git') + message = "mv-project failed: destination path <#{expected_distination_path}> already exists." expect(logger).to receive(:error).with(message) expect(gl_projects.mv_project('already-exists.git')).to be_falsy From ac862490392b029ac4937188e02bdf09f4505869 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 19 Dec 2017 10:57:12 +0100 Subject: [PATCH 43/50] Don't link LFS-objects multiple times. If Unlinking a fork would fail somewhere after this, the LFS objects might still be linked. Which would cause issues when trying to destroy a project. --- app/services/projects/unlink_fork_service.rb | 2 +- .../bvl-fix-unlinking-with-lfs-objects.yml | 6 ++++++ .../projects/unlink_fork_service_spec.rb | 20 +++++++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/bvl-fix-unlinking-with-lfs-objects.yml diff --git a/app/services/projects/unlink_fork_service.rb b/app/services/projects/unlink_fork_service.rb index c499f384426..842fe4e09c4 100644 --- a/app/services/projects/unlink_fork_service.rb +++ b/app/services/projects/unlink_fork_service.rb @@ -5,7 +5,7 @@ module Projects if fork_source = @project.fork_source fork_source.lfs_objects.find_each do |lfs_object| - lfs_object.projects << @project + lfs_object.projects << @project unless lfs_object.projects.include?(@project) end refresh_forks_count(fork_source) diff --git a/changelogs/unreleased/bvl-fix-unlinking-with-lfs-objects.yml b/changelogs/unreleased/bvl-fix-unlinking-with-lfs-objects.yml new file mode 100644 index 00000000000..058d686e74c --- /dev/null +++ b/changelogs/unreleased/bvl-fix-unlinking-with-lfs-objects.yml @@ -0,0 +1,6 @@ +--- +title: Don't link LFS objects to a project when unlinking forks when they were already + linked +merge_request: 16006 +author: +type: fixed diff --git a/spec/services/projects/unlink_fork_service_spec.rb b/spec/services/projects/unlink_fork_service_spec.rb index 2bba71fef4f..3ec6139bfa6 100644 --- a/spec/services/projects/unlink_fork_service_spec.rb +++ b/spec/services/projects/unlink_fork_service_spec.rb @@ -62,6 +62,26 @@ describe Projects::UnlinkForkService do expect(source.forks_count).to be_zero end + context 'when the source has LFS objects' do + let(:lfs_object) { create(:lfs_object) } + + before do + lfs_object.projects << project + end + + it 'links the fork to the lfs object before unlinking' do + subject.execute + + expect(lfs_object.projects).to include(forked_project) + end + + it 'does not fail if the lfs objects were already linked' do + lfs_object.projects << forked_project + + expect { subject.execute }.not_to raise_error + end + end + context 'when the original project was deleted' do it 'does not fail when the original project is deleted' do source = forked_project.forked_from_project From 23a38eac61f44c0ee3baed68ddcf23f28c03b20d Mon Sep 17 00:00:00 2001 From: Clement Ho Date: Tue, 19 Dec 2017 16:18:48 +0000 Subject: [PATCH 44/50] Fix shortcut links on help page --- app/assets/javascripts/docs/docs_bundle.js | 13 +++++++++++++ app/assets/javascripts/shortcuts.js | 10 ++++++++-- app/views/help/index.html.haml | 10 ++++++++-- .../unreleased/fix-docs-help-shortcut.yml | 5 +++++ config/webpack.config.js | 1 + spec/features/help_pages_spec.rb | 18 ++++++++++++++++++ 6 files changed, 53 insertions(+), 4 deletions(-) create mode 100644 app/assets/javascripts/docs/docs_bundle.js create mode 100644 changelogs/unreleased/fix-docs-help-shortcut.yml diff --git a/app/assets/javascripts/docs/docs_bundle.js b/app/assets/javascripts/docs/docs_bundle.js new file mode 100644 index 00000000000..a32bd6d0fc7 --- /dev/null +++ b/app/assets/javascripts/docs/docs_bundle.js @@ -0,0 +1,13 @@ +import Mousetrap from 'mousetrap'; + +function addMousetrapClick(el, key) { + el.addEventListener('click', () => Mousetrap.trigger(key)); +} + +function domContentLoaded() { + addMousetrapClick(document.querySelector('.js-trigger-shortcut'), '?'); + addMousetrapClick(document.querySelector('.js-trigger-search-bar'), 's'); +} + +document.addEventListener('DOMContentLoaded', domContentLoaded); + diff --git a/app/assets/javascripts/shortcuts.js b/app/assets/javascripts/shortcuts.js index 130730b1700..d2f0d7410da 100644 --- a/app/assets/javascripts/shortcuts.js +++ b/app/assets/javascripts/shortcuts.js @@ -51,7 +51,10 @@ export default class Shortcuts { } onToggleHelp(e) { - e.preventDefault(); + if (e.preventDefault) { + e.preventDefault(); + } + Shortcuts.toggleHelp(this.enabledHelp); } @@ -112,6 +115,9 @@ export default class Shortcuts { static focusSearch(e) { $('#search').focus(); - e.preventDefault(); + + if (e.preventDefault) { + e.preventDefault(); + } } } diff --git a/app/views/help/index.html.haml b/app/views/help/index.html.haml index 021de4f0caf..b8692009225 100644 --- a/app/views/help/index.html.haml +++ b/app/views/help/index.html.haml @@ -1,3 +1,5 @@ += webpack_bundle_tag 'docs' + %div - if current_application_settings.help_page_text.present? = markdown_field(current_application_settings, :help_page_text) @@ -37,8 +39,12 @@ Quick help %ul.well-list %li= link_to 'See our website for getting help', support_url - %li= link_to 'Use the search bar on the top of this page', '#', onclick: 'Shortcuts.focusSearch(event)' - %li= link_to 'Use shortcuts', '#', onclick: 'Shortcuts.toggleHelp()' + %li + %button.btn-blank.btn-link.js-trigger-search-bar{ type: 'button' } + Use the search bar on the top of this page + %li + %button.btn-blank.btn-link.js-trigger-shortcut{ type: 'button' } + Use shortcuts - unless current_application_settings.help_page_hide_commercial_content? %li= link_to 'Get a support subscription', 'https://about.gitlab.com/pricing/' %li= link_to 'Compare GitLab editions', 'https://about.gitlab.com/features/#compare' diff --git a/changelogs/unreleased/fix-docs-help-shortcut.yml b/changelogs/unreleased/fix-docs-help-shortcut.yml new file mode 100644 index 00000000000..8c172e44160 --- /dev/null +++ b/changelogs/unreleased/fix-docs-help-shortcut.yml @@ -0,0 +1,5 @@ +--- +title: Fix shortcut links on help page +merge_request: +author: +type: fixed diff --git a/config/webpack.config.js b/config/webpack.config.js index 78ced4c3e8c..f02fcda827a 100644 --- a/config/webpack.config.js +++ b/config/webpack.config.js @@ -36,6 +36,7 @@ var config = { cycle_analytics: './cycle_analytics/cycle_analytics_bundle.js', commit_pipelines: './commit/pipelines/pipelines_bundle.js', deploy_keys: './deploy_keys/index.js', + docs: './docs/docs_bundle.js', diff_notes: './diff_notes/diff_notes_bundle.js', environments: './environments/environments_bundle.js', environments_folder: './environments/folder/environments_folder_bundle.js', diff --git a/spec/features/help_pages_spec.rb b/spec/features/help_pages_spec.rb index ab896a310be..0d04ed612c2 100644 --- a/spec/features/help_pages_spec.rb +++ b/spec/features/help_pages_spec.rb @@ -32,6 +32,24 @@ describe 'Help Pages' do it_behaves_like 'help page', prefix: '/gitlab' end + + context 'quick link shortcuts', :js do + before do + visit help_path + end + + it 'focuses search bar' do + find('.js-trigger-search-bar').click + + expect(page).to have_selector('#search:focus') + end + + it 'opens shortcuts help dialog' do + find('.js-trigger-shortcut').click + + expect(page).to have_selector('#modal-shortcuts') + end + end end context 'in a production environment with version check enabled', :js do From 9410fbef9fa3979673b3769c5dc40686ea125240 Mon Sep 17 00:00:00 2001 From: Mario de la Ossa Date: Tue, 19 Dec 2017 17:02:56 +0000 Subject: [PATCH 45/50] Fix tags in the Activity tab not being clickable --- app/views/events/event/_push.html.haml | 3 +- .../unreleased/33028-event-tag-links.yml | 5 ++ .../events/event/_push.html.haml_spec.rb | 55 +++++++++++++++++++ 3 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/33028-event-tag-links.yml create mode 100644 spec/views/events/event/_push.html.haml_spec.rb diff --git a/app/views/events/event/_push.html.haml b/app/views/events/event/_push.html.haml index 9a763887b30..f85f5c5be88 100644 --- a/app/views/events/event/_push.html.haml +++ b/app/views/events/event/_push.html.haml @@ -7,7 +7,8 @@ %span.pushed #{event.action_name} #{event.ref_type} %strong - commits_link = project_commits_path(project, event.ref_name) - = link_to_if project.repository.branch_exists?(event.ref_name), event.ref_name, commits_link, class: 'ref-name' + - should_link = event.tag? ? project.repository.tag_exists?(event.ref_name) : project.repository.branch_exists?(event.ref_name) + = link_to_if should_link, event.ref_name, commits_link, class: 'ref-name' = render "events/event_scope", event: event diff --git a/changelogs/unreleased/33028-event-tag-links.yml b/changelogs/unreleased/33028-event-tag-links.yml new file mode 100644 index 00000000000..1d674200dcd --- /dev/null +++ b/changelogs/unreleased/33028-event-tag-links.yml @@ -0,0 +1,5 @@ +--- +title: Fix tags in the Activity tab not being clickable +merge_request: 15996 +author: Mario de la Ossa +type: fixed diff --git a/spec/views/events/event/_push.html.haml_spec.rb b/spec/views/events/event/_push.html.haml_spec.rb new file mode 100644 index 00000000000..f5634de4916 --- /dev/null +++ b/spec/views/events/event/_push.html.haml_spec.rb @@ -0,0 +1,55 @@ +require 'spec_helper' + +describe 'events/event/_push.html.haml' do + let(:event) { build_stubbed(:push_event) } + + context 'with a branch' do + let(:payload) { build_stubbed(:push_event_payload, event: event) } + + before do + allow(event).to receive(:push_event_payload).and_return(payload) + end + + it 'links to the branch' do + allow(event.project.repository).to receive(:branch_exists?).with(event.ref_name).and_return(true) + link = project_commits_path(event.project, event.ref_name) + + render partial: 'events/event/push', locals: { event: event } + + expect(rendered).to have_link(event.ref_name, href: link) + end + + context 'that has been deleted' do + it 'does not link to the branch' do + render partial: 'events/event/push', locals: { event: event } + + expect(rendered).not_to have_link(event.ref_name) + end + end + end + + context 'with a tag' do + let(:payload) { build_stubbed(:push_event_payload, event: event, ref_type: :tag, ref: 'v0.1.0') } + + before do + allow(event).to receive(:push_event_payload).and_return(payload) + end + + it 'links to the tag' do + allow(event.project.repository).to receive(:tag_exists?).with(event.ref_name).and_return(true) + link = project_commits_path(event.project, event.ref_name) + + render partial: 'events/event/push', locals: { event: event } + + expect(rendered).to have_link(event.ref_name, href: link) + end + + context 'that has been deleted' do + it 'does not link to the tag' do + render partial: 'events/event/push', locals: { event: event } + + expect(rendered).not_to have_link(event.ref_name) + end + end + end +end From 271ebe661103b4b523ae9e50f755df9a7aeedb1d Mon Sep 17 00:00:00 2001 From: Clement Ho Date: Tue, 19 Dec 2017 17:48:29 +0000 Subject: [PATCH 46/50] Show inline edit button for issues --- app/assets/javascripts/issue_show/components/app.vue | 2 +- app/assets/javascripts/issue_show/components/title.vue | 2 +- app/assets/javascripts/issue_show/index.js | 7 ------- app/views/projects/issues/show.html.haml | 5 ----- changelogs/unreleased/show-inline-edit-btn.yml | 5 +++++ spec/features/issues/issue_detail_spec.rb | 2 +- spec/features/projects/issuable_templates_spec.rb | 8 ++------ 7 files changed, 10 insertions(+), 21 deletions(-) create mode 100644 changelogs/unreleased/show-inline-edit-btn.yml diff --git a/app/assets/javascripts/issue_show/components/app.vue b/app/assets/javascripts/issue_show/components/app.vue index 25ebe5314e0..952f49d522e 100644 --- a/app/assets/javascripts/issue_show/components/app.vue +++ b/app/assets/javascripts/issue_show/components/app.vue @@ -32,7 +32,7 @@ export default { showInlineEditButton: { type: Boolean, required: false, - default: false, + default: true, }, showDeleteButton: { type: Boolean, diff --git a/app/assets/javascripts/issue_show/components/title.vue b/app/assets/javascripts/issue_show/components/title.vue index a363d06d950..b7e6eadd440 100644 --- a/app/assets/javascripts/issue_show/components/title.vue +++ b/app/assets/javascripts/issue_show/components/title.vue @@ -79,7 +79,7 @@ v-tooltip v-if="showInlineEditButton && canUpdate" type="button" - class="btn btn-default btn-edit btn-svg" + class="btn btn-default btn-edit btn-svg js-issuable-edit" v-html="pencilIcon" title="Edit title and description" data-placement="bottom" diff --git a/app/assets/javascripts/issue_show/index.js b/app/assets/javascripts/issue_show/index.js index 7b762496ba5..75dfdedcf1b 100644 --- a/app/assets/javascripts/issue_show/index.js +++ b/app/assets/javascripts/issue_show/index.js @@ -1,5 +1,4 @@ import Vue from 'vue'; -import eventHub from './event_hub'; import issuableApp from './components/app.vue'; import '../vue_shared/vue_resource_interceptor'; @@ -7,12 +6,6 @@ document.addEventListener('DOMContentLoaded', () => { const initialDataEl = document.getElementById('js-issuable-app-initial-data'); const props = JSON.parse(initialDataEl.innerHTML.replace(/"/g, '"')); - $('.js-issuable-edit').on('click', (e) => { - e.preventDefault(); - - eventHub.$emit('open.form'); - }); - return new Vue({ el: document.getElementById('js-issuable-app'), components: { diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml index eab7879c7bf..1f28d8acff6 100644 --- a/app/views/projects/issues/show.html.haml +++ b/app/views/projects/issues/show.html.haml @@ -39,8 +39,6 @@ = icon('caret-down') .dropdown-menu.dropdown-menu-align-right.hidden-lg %ul - - if can_update_issue - %li= link_to 'Edit', edit_project_issue_path(@project, @issue), class: 'js-issuable-edit' - unless current_user == @issue.author %li= link_to 'Report abuse', new_abuse_report_path(user_id: @issue.author.id, ref_url: issue_url(@issue)) - if can_update_issue @@ -52,9 +50,6 @@ %li.divider %li= link_to 'New issue', new_project_issue_path(@project), title: 'New issue', id: 'new_issue_link' - - if can_update_issue - = link_to 'Edit', edit_project_issue_path(@project, @issue), class: 'hidden-xs hidden-sm btn btn-grouped js-issuable-edit' - = render 'shared/issuable/close_reopen_button', issuable: @issue, can_update: can_update_issue - if can_report_spam diff --git a/changelogs/unreleased/show-inline-edit-btn.yml b/changelogs/unreleased/show-inline-edit-btn.yml new file mode 100644 index 00000000000..8cfe9b7d75a --- /dev/null +++ b/changelogs/unreleased/show-inline-edit-btn.yml @@ -0,0 +1,5 @@ +--- +title: Move edit button to second row on issue page (and change it to a pencil icon) +merge_request: +author: +type: changed diff --git a/spec/features/issues/issue_detail_spec.rb b/spec/features/issues/issue_detail_spec.rb index 4224a8fe5d4..babb0285590 100644 --- a/spec/features/issues/issue_detail_spec.rb +++ b/spec/features/issues/issue_detail_spec.rb @@ -24,7 +24,7 @@ feature 'Issue Detail', :js do visit project_issue_path(project, issue) wait_for_requests - click_link 'Edit' + page.find('.js-issuable-edit').click fill_in 'issuable-title', with: 'issue title' click_button 'Save' wait_for_requests diff --git a/spec/features/projects/issuable_templates_spec.rb b/spec/features/projects/issuable_templates_spec.rb index 0257cd157c9..4319fc2746c 100644 --- a/spec/features/projects/issuable_templates_spec.rb +++ b/spec/features/projects/issuable_templates_spec.rb @@ -32,9 +32,7 @@ feature 'issuable templates', :js do message: 'added issue template', branch_name: 'master') visit project_issue_path project, issue - page.within('.js-issuable-actions') do - click_on 'Edit' - end + page.find('.js-issuable-edit').click fill_in :'issuable-title', with: 'test issue title' end @@ -77,9 +75,7 @@ feature 'issuable templates', :js do message: 'added issue template', branch_name: 'master') visit project_issue_path project, issue - page.within('.js-issuable-actions') do - click_on 'Edit' - end + page.find('.js-issuable-edit').click fill_in :'issuable-title', with: 'test issue title' fill_in :'issue-description', with: prior_description end From 0a0a62c93636627e2c1c34a41af33e178efd7647 Mon Sep 17 00:00:00 2001 From: Clement Ho Date: Tue, 19 Dec 2017 17:50:38 +0000 Subject: [PATCH 47/50] Remove related links in MR widget when empty state --- .../mr_widget_options.js | 2 +- .../stores/get_state_key.js | 28 ++++++++++--------- .../stores/mr_widget_store.js | 5 ++++ .../stores/state_maps.js | 17 +++++++++++ .../remove-links-mr-empty-state.yml | 5 ++++ .../vue_mr_widget/mr_widget_options_spec.js | 28 +++++++++++++++++++ .../stores/mr_widget_store_spec.js | 13 +++++++++ 7 files changed, 84 insertions(+), 14 deletions(-) create mode 100644 changelogs/unreleased/remove-links-mr-empty-state.yml diff --git a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js index 9cb3edead86..8a9129c385b 100644 --- a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js +++ b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js @@ -62,7 +62,7 @@ export default { return this.mr.hasCI; }, shouldRenderRelatedLinks() { - return !!this.mr.relatedLinks; + return !!this.mr.relatedLinks && !this.mr.isNothingToMergeState; }, shouldRenderDeployments() { return this.mr.deployments.length; diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js b/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js index 7c15abfff10..2bace3311c8 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js @@ -1,30 +1,32 @@ +import { stateKey } from './state_maps'; + export default function deviseState(data) { if (data.project_archived) { - return 'archived'; + return stateKey.archived; } else if (data.branch_missing) { - return 'missingBranch'; + return stateKey.missingBranch; } else if (!data.commits_count) { - return 'nothingToMerge'; + return stateKey.nothingToMerge; } else if (this.mergeStatus === 'unchecked') { - return 'checking'; + return stateKey.checking; } else if (data.has_conflicts) { - return 'conflicts'; + return stateKey.conflicts; } else if (data.work_in_progress) { - return 'workInProgress'; + return stateKey.workInProgress; } else if (this.onlyAllowMergeIfPipelineSucceeds && this.isPipelineFailed) { - return 'pipelineFailed'; + return stateKey.pipelineFailed; } else if (this.hasMergeableDiscussionsState) { - return 'unresolvedDiscussions'; + return stateKey.unresolvedDiscussions; } else if (this.isPipelineBlocked) { - return 'pipelineBlocked'; + return stateKey.pipelineBlocked; } else if (this.hasSHAChanged) { - return 'shaMismatch'; + return stateKey.shaMismatch; } else if (this.mergeWhenPipelineSucceeds) { - return this.mergeError ? 'autoMergeFailed' : 'mergeWhenPipelineSucceeds'; + return this.mergeError ? stateKey.autoMergeFailed : stateKey.mergeWhenPipelineSucceeds; } else if (!this.canMerge) { - return 'notAllowedToMerge'; + return stateKey.notAllowedToMerge; } else if (this.canBeMerged) { - return 'readyToMerge'; + return stateKey.readyToMerge; } return null; } diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js index 707766e08e4..93d31a2a684 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js @@ -1,5 +1,6 @@ import Timeago from 'timeago.js'; import { getStateKey } from '../dependencies'; +import { stateKey } from './state_maps'; import { formatDate } from '../../lib/utils/datetime_utility'; export default class MergeRequestStore { @@ -120,6 +121,10 @@ export default class MergeRequestStore { } } + get isNothingToMergeState() { + return this.state === stateKey.nothingToMerge; + } + static getEventObject(event) { return { author: MergeRequestStore.getAuthorObject(event), diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js b/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js index 9074a064a6d..de980c175fb 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js @@ -31,6 +31,23 @@ const statesToShowHelpWidget = [ 'autoMergeFailed', ]; +export const stateKey = { + archived: 'archived', + missingBranch: 'missingBranch', + nothingToMerge: 'nothingToMerge', + checking: 'checking', + conflicts: 'conflicts', + workInProgress: 'workInProgress', + pipelineFailed: 'pipelineFailed', + unresolvedDiscussions: 'unresolvedDiscussions', + pipelineBlocked: 'pipelineBlocked', + shaMismatch: 'shaMismatch', + autoMergeFailed: 'autoMergeFailed', + mergeWhenPipelineSucceeds: 'mergeWhenPipelineSucceeds', + notAllowedToMerge: 'notAllowedToMerge', + readyToMerge: 'readyToMerge', +}; + export default { stateToComponentMap, statesToShowHelpWidget, diff --git a/changelogs/unreleased/remove-links-mr-empty-state.yml b/changelogs/unreleased/remove-links-mr-empty-state.yml new file mode 100644 index 00000000000..c666bc2c81d --- /dev/null +++ b/changelogs/unreleased/remove-links-mr-empty-state.yml @@ -0,0 +1,5 @@ +--- +title: Remove related links in MR widget when empty state +merge_request: +author: +type: fixed diff --git a/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js b/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js index 9e6d0aa472c..74b343c573e 100644 --- a/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js +++ b/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js @@ -2,6 +2,7 @@ import Vue from 'vue'; import mrWidgetOptions from '~/vue_merge_request_widget/mr_widget_options'; import eventHub from '~/vue_merge_request_widget/event_hub'; import notify from '~/lib/utils/notify'; +import { stateKey } from '~/vue_merge_request_widget/stores/state_maps'; import mockData from './mock_data'; import mountComponent from '../helpers/vue_mount_component_helper'; @@ -344,4 +345,31 @@ describe('mrWidgetOptions', () => { expect(comps['mr-widget-merge-when-pipeline-succeeds']).toBeDefined(); }); }); + + describe('rendering relatedLinks', () => { + beforeEach((done) => { + vm.mr.relatedLinks = { + assignToMe: null, + closing: ` + Date: Tue, 19 Dec 2017 18:36:53 +0000 Subject: [PATCH 50/50] Prevent some specs from mangling the gitlab-shell checkout --- ...s_link_to_create_license_file_in_empty_project_spec.rb | 8 +++----- spec/features/tags/master_views_tags_spec.rb | 5 ++--- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/spec/features/projects/files/project_owner_sees_link_to_create_license_file_in_empty_project_spec.rb b/spec/features/projects/files/project_owner_sees_link_to_create_license_file_in_empty_project_spec.rb index 6c616bf0456..8ac9821b879 100644 --- a/spec/features/projects/files/project_owner_sees_link_to_create_license_file_in_empty_project_spec.rb +++ b/spec/features/projects/files/project_owner_sees_link_to_create_license_file_in_empty_project_spec.rb @@ -2,15 +2,15 @@ require 'spec_helper' feature 'project owner sees a link to create a license file in empty project', :js do let(:project_master) { create(:user) } - let(:project) { create(:project) } + let(:project) { create(:project_empty_repo) } + background do - project.team << [project_master, :master] + project.add_master(project_master) sign_in(project_master) end scenario 'project master creates a license file from a template' do visit project_path(project) - click_link 'Create empty bare repository' click_on 'LICENSE' expect(page).to have_content('New file') @@ -26,8 +26,6 @@ feature 'project owner sees a link to create a license file in empty project', : expect(file_content).to have_content("Copyright (c) #{Time.now.year} #{project.namespace.human_name}") fill_in :commit_message, with: 'Add a LICENSE file', visible: true - # Remove pre-receive hook so we can push without auth - FileUtils.rm_f(File.join(project.repository.path, 'hooks', 'pre-receive')) click_button 'Commit changes' expect(current_path).to eq( diff --git a/spec/features/tags/master_views_tags_spec.rb b/spec/features/tags/master_views_tags_spec.rb index 9edc7ced163..4662367d843 100644 --- a/spec/features/tags/master_views_tags_spec.rb +++ b/spec/features/tags/master_views_tags_spec.rb @@ -4,18 +4,17 @@ feature 'Master views tags' do let(:user) { create(:user) } before do - project.team << [user, :master] + project.add_master(user) sign_in(user) end context 'when project has no tags' do let(:project) { create(:project_empty_repo) } + before do visit project_path(project) click_on 'README' fill_in :commit_message, with: 'Add a README file', visible: true - # Remove pre-receive hook so we can push without auth - FileUtils.rm_f(File.join(project.repository.path, 'hooks', 'pre-receive')) click_button 'Commit changes' visit project_tags_path(project) end