From f7edc65899aa2d360d26215b13212f681b8f0b95 Mon Sep 17 00:00:00 2001 From: Jonathon Reinhart Date: Fri, 22 Sep 2017 01:57:13 +0000 Subject: Update installation.md with receive.advertisePushOptions See https://gitlab.com/gitlab-org/gitlab-ce/issues/33061#note_37473342 --- doc/install/installation.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/install/installation.md b/doc/install/installation.md index 200cd94f43c..7eec3132393 100644 --- a/doc/install/installation.md +++ b/doc/install/installation.md @@ -367,6 +367,9 @@ sudo usermod -aG redis git # Enable packfile bitmaps sudo -u git -H git config --global repack.writeBitmaps true + + # Enable push options + sudo -u git -H git config --global receive.advertisePushOptions true # Configure Redis connection settings sudo -u git -H cp config/resque.yml.example config/resque.yml -- cgit v1.2.1 From 9ad48a7438048ca19ff847e5fa7f2ccd7c6d59af Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 9 Oct 2017 13:07:55 +0200 Subject: Extract class responsible for building a pipeline --- app/services/ci/create_pipeline_service.rb | 55 +++++++---------------------- lib/gitlab/ci/pipeline/chain/build.rb | 56 ++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 43 deletions(-) create mode 100644 lib/gitlab/ci/pipeline/chain/build.rb diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index 31a712ccc1b..7b9ea223d26 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -2,27 +2,24 @@ module Ci class CreatePipelineService < BaseService attr_reader :pipeline - SEQUENCE = [Gitlab::Ci::Pipeline::Chain::Validate::Abilities, + SEQUENCE = [Gitlab::Ci::Pipeline::Chain::Build, + Gitlab::Ci::Pipeline::Chain::Validate::Abilities, Gitlab::Ci::Pipeline::Chain::Validate::Repository, Gitlab::Ci::Pipeline::Chain::Validate::Config, Gitlab::Ci::Pipeline::Chain::Skip, Gitlab::Ci::Pipeline::Chain::Create].freeze def execute(source, ignore_skip_ci: false, save_on_errors: true, trigger_request: nil, schedule: nil, &block) - @pipeline = Ci::Pipeline.new( - source: source, - project: project, - ref: ref, - sha: sha, - before_sha: before_sha, - tag: tag_exists?, - trigger_requests: Array(trigger_request), - user: current_user, - pipeline_schedule: schedule, - protected: project.protected_for?(ref) - ) - - command = OpenStruct.new(ignore_skip_ci: ignore_skip_ci, + @pipeline = Ci::Pipeline.new + + command = OpenStruct.new(source: source, + origin_ref: params[:ref], + checkout_sha: params[:checkout_sha], + after_sha: params[:after], + before_sha: params[:before], + trigger_request: trigger_request, + schedule: schedule, + ignore_skip_ci: ignore_skip_ci, save_incompleted: save_on_errors, seeds_block: block, project: project, @@ -45,14 +42,6 @@ module Ci private - def commit - @commit ||= project.commit(origin_sha || origin_ref) - end - - def sha - commit.try(:id) - end - def update_merge_requests_head_pipeline return unless pipeline.latest? @@ -76,26 +65,6 @@ module Ci .created_or_pending end - def before_sha - params[:checkout_sha] || params[:before] || Gitlab::Git::BLANK_SHA - end - - def origin_sha - params[:checkout_sha] || params[:after] - end - - def origin_ref - params[:ref] - end - - def tag_exists? - project.repository.tag_exists?(ref) - end - - def ref - @ref ||= Gitlab::Git.ref_name(origin_ref) - end - def pipeline_created_counter @pipeline_created_counter ||= Gitlab::Metrics .counter(:pipelines_created_total, "Counter of pipelines created") diff --git a/lib/gitlab/ci/pipeline/chain/build.rb b/lib/gitlab/ci/pipeline/chain/build.rb new file mode 100644 index 00000000000..efd1da733c5 --- /dev/null +++ b/lib/gitlab/ci/pipeline/chain/build.rb @@ -0,0 +1,56 @@ +module Gitlab + module Ci + module Pipeline + module Chain + class Build < Chain::Base + include Chain::Helpers + + def perform! + @pipeline.assign_attributes( + source: @command.source, + project: @project, + ref: ref, + sha: sha, + before_sha: before_sha, + tag: tag_exists?, + trigger_requests: Array(@command.trigger_request), + user: @current_user, + pipeline_schedule: @command.schedule, + protected: protected_ref? + ) + end + + def break? + false + end + + private + + def ref + @ref ||= Gitlab::Git.ref_name(origin_ref) + end + + def sha + @project.commit(origin_sha || origin_ref).try(:id) + end + + def origin_ref + @command.origin_ref + end + + def origin_sha + @command.checkout_sha || @command.after_sha + end + + def before_sha + @command.checkout_sha || @command.before_sha || Gitlab::Git::BLANK_SHA + end + + def protected_ref? + @project.protected_for?(ref) + end + end + end + end + end +end -- cgit v1.2.1 From 20fc42e99a5e8c917feaa0c7a2d34aceba4a8eb9 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 10 Oct 2017 10:11:19 +0200 Subject: Improve post_receive spec by not stubbing private methods --- spec/workers/post_receive_spec.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb index 05eecf5f0bb..5d9b0679796 100644 --- a/spec/workers/post_receive_spec.rb +++ b/spec/workers/post_receive_spec.rb @@ -66,19 +66,21 @@ describe PostReceive do end context "gitlab-ci.yml" do + let(:changes) { "123456 789012 refs/heads/feature\n654321 210987 refs/tags/tag" } + subject { described_class.new.perform(gl_repository, key_id, base64_changes) } context "creates a Ci::Pipeline for every change" do before do stub_ci_pipeline_to_return_yaml_file - # TODO, don't stub private methods - # - allow_any_instance_of(Ci::CreatePipelineService) - .to receive(:commit).and_return(OpenStruct.new(id: '123456')) + allow_any_instance_of(Project) + .to receive(:commit) + .and_return(project.commit) allow_any_instance_of(Repository) - .to receive(:branch_exists?).and_return(true) + .to receive(:branch_exists?) + .and_return(true) end it { expect { subject }.to change { Ci::Pipeline.count }.by(2) } -- cgit v1.2.1 From 54f490a455dea705908d5e262fb5fe205147fa09 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 10 Oct 2017 14:00:56 +0200 Subject: Add initial specs for pipeline build chain class --- spec/lib/gitlab/ci/pipeline/chain/build_spec.rb | 43 +++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 spec/lib/gitlab/ci/pipeline/chain/build_spec.rb diff --git a/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb new file mode 100644 index 00000000000..fbb72e6a51a --- /dev/null +++ b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb @@ -0,0 +1,43 @@ +require 'spec_helper' + +describe Gitlab::Ci::Pipeline::Chain::Build do + set(:project) { create(:project, :repository) } + set(:user) { create(:user) } + let(:pipeline) { Ci::Pipeline.new } + + let(:command) do + double('command', source: :push, + origin_ref: 'master', + checkout_sha: project.commit.id, + after_sha: nil, + before_sha: nil, + trigger_request: nil, + schedule: nil, + project: project, + current_user: user) + end + + let(:step) { described_class.new(pipeline, command) } + + before do + stub_ci_pipeline_to_return_yaml_file + + step.perform! + end + + it 'never breaks the chain' do + expect(step.break?).to be false + end + + it 'fills pipeline object with data' do + expect(pipeline.sha).not_to be_empty + end + + it 'sets a valid config source' do + expect(pipeline.repository_source?).to be true + end + + it 'returns a valid pipeline' do + expect(pipeline).to be_valid + end +end -- cgit v1.2.1 From eedf43d69bb2d3e5ed73b751565beb5d1badd8c7 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 11 Oct 2017 14:48:28 +0200 Subject: Set pipeline config source attribute in a build step --- lib/gitlab/ci/pipeline/chain/build.rb | 2 ++ spec/lib/gitlab/ci/pipeline/chain/build_spec.rb | 2 +- spec/services/ci/create_pipeline_service_spec.rb | 3 ++- spec/support/stub_gitlab_calls.rb | 6 ++++++ 4 files changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/ci/pipeline/chain/build.rb b/lib/gitlab/ci/pipeline/chain/build.rb index efd1da733c5..a126dded1ae 100644 --- a/lib/gitlab/ci/pipeline/chain/build.rb +++ b/lib/gitlab/ci/pipeline/chain/build.rb @@ -18,6 +18,8 @@ module Gitlab pipeline_schedule: @command.schedule, protected: protected_ref? ) + + @pipeline.set_config_source end def break? diff --git a/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb index fbb72e6a51a..82f70dd176d 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb @@ -20,7 +20,7 @@ describe Gitlab::Ci::Pipeline::Chain::Build do let(:step) { described_class.new(pipeline, command) } before do - stub_ci_pipeline_to_return_yaml_file + stub_repository_ci_yaml_file(sha: anything) step.perform! end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 08847183bf4..befd0faf1b6 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -8,7 +8,7 @@ describe Ci::CreatePipelineService do let(:ref_name) { 'refs/heads/master' } before do - stub_ci_pipeline_to_return_yaml_file + stub_repository_ci_yaml_file(sha: anything) end describe '#execute' do @@ -44,6 +44,7 @@ describe Ci::CreatePipelineService do expect(pipeline).to eq(project.pipelines.last) expect(pipeline).to have_attributes(user: user) expect(pipeline).to have_attributes(status: 'pending') + expect(pipeline.repository_source?).to be true expect(pipeline.builds.first).to be_kind_of(Ci::Build) end diff --git a/spec/support/stub_gitlab_calls.rb b/spec/support/stub_gitlab_calls.rb index 5f22d886910..c1618f5086c 100644 --- a/spec/support/stub_gitlab_calls.rb +++ b/spec/support/stub_gitlab_calls.rb @@ -21,6 +21,12 @@ module StubGitlabCalls allow_any_instance_of(Ci::Pipeline).to receive(:ci_yaml_file) { ci_yaml } end + def stub_repository_ci_yaml_file(sha:, path: '.gitlab-ci.yml') + allow_any_instance_of(Repository) + .to receive(:gitlab_ci_yml_for).with(sha, path) + .and_return(gitlab_ci_yaml) + end + def stub_ci_builds_disabled allow_any_instance_of(Project).to receive(:builds_enabled?).and_return(false) end -- cgit v1.2.1 From 1486950bc9396f9b8384763d68f36387326eb745 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 11 Oct 2017 14:51:39 +0200 Subject: Extend pipeline build chain specs --- spec/lib/gitlab/ci/pipeline/chain/build_spec.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb index 82f70dd176d..0f1d72080c5 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/build_spec.rb @@ -31,6 +31,10 @@ describe Gitlab::Ci::Pipeline::Chain::Build do it 'fills pipeline object with data' do expect(pipeline.sha).not_to be_empty + expect(pipeline.sha).to eq project.commit.id + expect(pipeline.ref).to eq 'master' + expect(pipeline.user).to eq user + expect(pipeline.project).to eq project end it 'sets a valid config source' do @@ -40,4 +44,8 @@ describe Gitlab::Ci::Pipeline::Chain::Build do it 'returns a valid pipeline' do expect(pipeline).to be_valid end + + it 'does not persist a pipeline' do + expect(pipeline).not_to be_persisted + end end -- cgit v1.2.1 From f4ed780ef5fc76b7704742d4886ac435c3e5ab98 Mon Sep 17 00:00:00 2001 From: George Andrinopoulos Date: Mon, 6 Nov 2017 19:51:24 +0200 Subject: Add Group Milestone sorting --- app/controllers/groups/milestones_controller.rb | 3 +- app/views/groups/milestones/index.html.haml | 1 + .../_group_milestones_sort_dropdown.html.haml | 22 +++++++++++ .../unreleased/39720-group-milestone-sorting.yml | 5 +++ lib/milestone_array.rb | 40 +++++++++++++++++++ spec/features/groups/milestones_sorting_spec.rb | 46 ++++++++++++++++++++++ spec/lib/milestone_array_spec.rb | 34 ++++++++++++++++ 7 files changed, 150 insertions(+), 1 deletion(-) create mode 100644 app/views/shared/_group_milestones_sort_dropdown.html.haml create mode 100644 changelogs/unreleased/39720-group-milestone-sorting.yml create mode 100644 lib/milestone_array.rb create mode 100644 spec/features/groups/milestones_sorting_spec.rb create mode 100644 spec/lib/milestone_array_spec.rb diff --git a/app/controllers/groups/milestones_controller.rb b/app/controllers/groups/milestones_controller.rb index 7a7bcb1a3d2..f013d21275e 100644 --- a/app/controllers/groups/milestones_controller.rb +++ b/app/controllers/groups/milestones_controller.rb @@ -80,7 +80,8 @@ class Groups::MilestonesController < Groups::ApplicationController milestones = MilestonesFinder.new(search_params).execute legacy_milestones = GroupMilestone.build_collection(group, group_projects, params) - milestones + legacy_milestones + @sort = params[:sort] || 'due_date_asc' + MilestoneArray.sort(milestones + legacy_milestones, @sort) end def milestone diff --git a/app/views/groups/milestones/index.html.haml b/app/views/groups/milestones/index.html.haml index cb4fc69d5b8..6c199c52583 100644 --- a/app/views/groups/milestones/index.html.haml +++ b/app/views/groups/milestones/index.html.haml @@ -4,6 +4,7 @@ = render 'shared/milestones_filter', counts: @milestone_states .nav-controls + = render 'shared/group_milestones_sort_dropdown' - if can?(current_user, :admin_milestones, @group) = link_to "New milestone", new_group_milestone_path(@group), class: "btn btn-new" diff --git a/app/views/shared/_group_milestones_sort_dropdown.html.haml b/app/views/shared/_group_milestones_sort_dropdown.html.haml new file mode 100644 index 00000000000..9b2f2fdcc93 --- /dev/null +++ b/app/views/shared/_group_milestones_sort_dropdown.html.haml @@ -0,0 +1,22 @@ +.dropdown.inline.prepend-left-10 + %button.dropdown-toggle{ type: 'button', data: { toggle: 'dropdown' } } + %span.light + - if @sort.present? + = milestone_sort_options_hash[@sort] + - else + = sort_title_due_date_soon + = icon('chevron-down') + %ul.dropdown-menu.dropdown-menu-align-right.dropdown-menu-sort + %li + = link_to page_filter_path(sort: sort_value_due_date_soon, label: true) do + = sort_title_due_date_soon + = link_to page_filter_path(sort: sort_value_due_date_later, label: true) do + = sort_title_due_date_later + = link_to page_filter_path(sort: sort_value_start_date_soon, label: true) do + = sort_title_start_date_soon + = link_to page_filter_path(sort: sort_value_start_date_later, label: true) do + = sort_title_start_date_later + = link_to page_filter_path(sort: sort_value_name, label: true) do + = sort_title_name_asc + = link_to page_filter_path(sort: sort_value_name_desc, label: true) do + = sort_title_name_desc diff --git a/changelogs/unreleased/39720-group-milestone-sorting.yml b/changelogs/unreleased/39720-group-milestone-sorting.yml new file mode 100644 index 00000000000..15ef87fa567 --- /dev/null +++ b/changelogs/unreleased/39720-group-milestone-sorting.yml @@ -0,0 +1,5 @@ +--- +title: Add dropdown sort to group milestones +merge_request: 15230 +author: George Andrinopoulos +type: added diff --git a/lib/milestone_array.rb b/lib/milestone_array.rb new file mode 100644 index 00000000000..9b3f2acc123 --- /dev/null +++ b/lib/milestone_array.rb @@ -0,0 +1,40 @@ +module MilestoneArray + class << self + def sort(array, sort_method) + case sort_method + when 'due_date_asc' + sort_asc_nulls_last(array, 'due_date') + when 'due_date_desc' + sort_desc_nulls_last(array, 'due_date') + when 'start_date_asc' + sort_asc_nulls_last(array, 'start_date') + when 'start_date_desc' + sort_desc_nulls_last(array, 'start_date') + when 'name_asc' + sort_asc(array, 'title') + when 'name_desc' + sort_desc(array, 'title') + else + array + end + end + + private + + def sort_asc_nulls_last(array, attribute) + array.select(&attribute.to_sym).sort_by(&attribute.to_sym) + array.reject(&attribute.to_sym) + end + + def sort_desc_nulls_last(array, attribute) + array.select(&attribute.to_sym).sort_by(&attribute.to_sym).reverse + array.reject(&attribute.to_sym) + end + + def sort_asc(array, attribute) + array.sort_by(&attribute.to_sym) + end + + def sort_desc(array, attribute) + array.sort_by(&attribute.to_sym).reverse + end + end +end diff --git a/spec/features/groups/milestones_sorting_spec.rb b/spec/features/groups/milestones_sorting_spec.rb new file mode 100644 index 00000000000..3bd59587535 --- /dev/null +++ b/spec/features/groups/milestones_sorting_spec.rb @@ -0,0 +1,46 @@ +require 'spec_helper' + +feature 'Milestones sorting', :js do + let(:group) { create(:group) } + let!(:project) { create(:project_empty_repo, group: group) } + let!(:other_project) { create(:project_empty_repo, group: group) } + let!(:project_milestone1) { create(:milestone, project: project, title: 'v1.0', due_date: 10.days.from_now) } + let!(:other_project_milestone1) { create(:milestone, project: other_project, title: 'v1.0', due_date: 10.days.from_now) } + let!(:project_milestone2) { create(:milestone, project: project, title: 'v2.0', due_date: 5.days.from_now) } + let!(:other_project_milestone2) { create(:milestone, project: other_project, title: 'v2.0', due_date: 5.days.from_now) } + let(:user) { create(:group_member, :master, user: create(:user), group: group ).user } + + before do + sign_in(user) + end + + scenario 'visit group milestones and sort by due_date_asc' do + visit group_milestones_path(group) + + expect(page).to have_button('Due soon') + + # assert default sorting + within '.milestones' do + expect(page.all('ul.content-list > li').first.text).to include('v2.0') + expect(page.all('ul.content-list > li').last.text).to include('v1.0') + end + + click_button 'Due soon' + + sort_options = find('ul.dropdown-menu-sort li').all('a').collect(&:text) + + expect(sort_options[0]).to eq('Due soon') + expect(sort_options[1]).to eq('Due later') + expect(sort_options[2]).to eq('Start soon') + expect(sort_options[3]).to eq('Start later') + + click_link 'Due later' + + expect(page).to have_button('Due later') + + within '.milestones' do + expect(page.all('ul.content-list > li').first.text).to include('v1.0') + expect(page.all('ul.content-list > li').last.text).to include('v2.0') + end + end +end diff --git a/spec/lib/milestone_array_spec.rb b/spec/lib/milestone_array_spec.rb new file mode 100644 index 00000000000..df91677b925 --- /dev/null +++ b/spec/lib/milestone_array_spec.rb @@ -0,0 +1,34 @@ +require 'spec_helper' + +describe MilestoneArray do + let(:object1) { instance_double("BirdMilestone", due_date: Time.now, start_date: Time.now - 15.days, title: 'v2.0') } + let(:object2) { instance_double("CatMilestone", due_date: Time.now - 1.day, start_date: nil, title: 'v1.0') } + let(:object3) { instance_double("DogMilestone", due_date: nil, start_date: Time.now - 30.days, title: 'v3.0') } + let(:array) { [object1, object3, object2] } + + describe '#sort' do + it 'reorders array with due date in ascending order with nulls last' do + expect(described_class.sort(array, 'due_date_asc')).to eq([object2, object1, object3]) + end + + it 'reorders array with due date in desc order with nulls last' do + expect(described_class.sort(array, 'due_date_desc')).to eq([object1, object2, object3]) + end + + it 'reorders array with start date in ascending order with nulls last' do + expect(described_class.sort(array, 'start_date_asc')).to eq([object3, object1, object2]) + end + + it 'reorders array with start date in descending order with nulls last' do + expect(described_class.sort(array, 'start_date_desc')).to eq([object1, object3, object2]) + end + + it 'reorders array with title in ascending order' do + expect(described_class.sort(array, 'name_asc')).to eq([object2, object1, object3]) + end + + it 'reorders array with title in descending order' do + expect(described_class.sort(array, 'name_desc')).to eq([object3, object1, object2]) + end + end +end -- cgit v1.2.1 From a4d71cba7ef80e6f3c10f148dd1edfbef7f82893 Mon Sep 17 00:00:00 2001 From: George Andrinopoulos Date: Tue, 7 Nov 2017 20:57:30 +0200 Subject: Add group milestone to feature spec and minor changes --- app/views/groups/milestones/index.html.haml | 2 +- .../_group_milestones_sort_dropdown.html.haml | 22 ---------------------- lib/milestone_array.rb | 14 +++++++------- spec/features/groups/milestones_sorting_spec.rb | 5 +++++ 4 files changed, 13 insertions(+), 30 deletions(-) delete mode 100644 app/views/shared/_group_milestones_sort_dropdown.html.haml diff --git a/app/views/groups/milestones/index.html.haml b/app/views/groups/milestones/index.html.haml index 6c199c52583..f5f621507b8 100644 --- a/app/views/groups/milestones/index.html.haml +++ b/app/views/groups/milestones/index.html.haml @@ -4,7 +4,7 @@ = render 'shared/milestones_filter', counts: @milestone_states .nav-controls - = render 'shared/group_milestones_sort_dropdown' + = render 'shared/milestones_sort_dropdown' - if can?(current_user, :admin_milestones, @group) = link_to "New milestone", new_group_milestone_path(@group), class: "btn btn-new" diff --git a/app/views/shared/_group_milestones_sort_dropdown.html.haml b/app/views/shared/_group_milestones_sort_dropdown.html.haml deleted file mode 100644 index 9b2f2fdcc93..00000000000 --- a/app/views/shared/_group_milestones_sort_dropdown.html.haml +++ /dev/null @@ -1,22 +0,0 @@ -.dropdown.inline.prepend-left-10 - %button.dropdown-toggle{ type: 'button', data: { toggle: 'dropdown' } } - %span.light - - if @sort.present? - = milestone_sort_options_hash[@sort] - - else - = sort_title_due_date_soon - = icon('chevron-down') - %ul.dropdown-menu.dropdown-menu-align-right.dropdown-menu-sort - %li - = link_to page_filter_path(sort: sort_value_due_date_soon, label: true) do - = sort_title_due_date_soon - = link_to page_filter_path(sort: sort_value_due_date_later, label: true) do - = sort_title_due_date_later - = link_to page_filter_path(sort: sort_value_start_date_soon, label: true) do - = sort_title_start_date_soon - = link_to page_filter_path(sort: sort_value_start_date_later, label: true) do - = sort_title_start_date_later - = link_to page_filter_path(sort: sort_value_name, label: true) do - = sort_title_name_asc - = link_to page_filter_path(sort: sort_value_name_desc, label: true) do - = sort_title_name_desc diff --git a/lib/milestone_array.rb b/lib/milestone_array.rb index 9b3f2acc123..4ed8485b36a 100644 --- a/lib/milestone_array.rb +++ b/lib/milestone_array.rb @@ -13,7 +13,7 @@ module MilestoneArray when 'name_asc' sort_asc(array, 'title') when 'name_desc' - sort_desc(array, 'title') + sort_asc(array, 'title').reverse else array end @@ -22,19 +22,19 @@ module MilestoneArray private def sort_asc_nulls_last(array, attribute) - array.select(&attribute.to_sym).sort_by(&attribute.to_sym) + array.reject(&attribute.to_sym) + attribute = attribute.to_sym + + array.select(&attribute).sort_by(&attribute) + array.reject(&attribute) end def sort_desc_nulls_last(array, attribute) - array.select(&attribute.to_sym).sort_by(&attribute.to_sym).reverse + array.reject(&attribute.to_sym) + attribute = attribute.to_sym + + array.select(&attribute).sort_by(&attribute).reverse + array.reject(&attribute) end def sort_asc(array, attribute) array.sort_by(&attribute.to_sym) end - - def sort_desc(array, attribute) - array.sort_by(&attribute.to_sym).reverse - end end end diff --git a/spec/features/groups/milestones_sorting_spec.rb b/spec/features/groups/milestones_sorting_spec.rb index 3bd59587535..a0fe40cf1d3 100644 --- a/spec/features/groups/milestones_sorting_spec.rb +++ b/spec/features/groups/milestones_sorting_spec.rb @@ -8,6 +8,7 @@ feature 'Milestones sorting', :js do let!(:other_project_milestone1) { create(:milestone, project: other_project, title: 'v1.0', due_date: 10.days.from_now) } let!(:project_milestone2) { create(:milestone, project: project, title: 'v2.0', due_date: 5.days.from_now) } let!(:other_project_milestone2) { create(:milestone, project: other_project, title: 'v2.0', due_date: 5.days.from_now) } + let!(:group_milestone) { create(:milestone, group: group, title: 'v3.0', due_date: 7.days.from_now) } let(:user) { create(:group_member, :master, user: create(:user), group: group ).user } before do @@ -22,6 +23,7 @@ feature 'Milestones sorting', :js do # assert default sorting within '.milestones' do expect(page.all('ul.content-list > li').first.text).to include('v2.0') + expect(page.all('ul.content-list > li')[1].text).to include('v3.0') expect(page.all('ul.content-list > li').last.text).to include('v1.0') end @@ -33,6 +35,8 @@ feature 'Milestones sorting', :js do expect(sort_options[1]).to eq('Due later') expect(sort_options[2]).to eq('Start soon') expect(sort_options[3]).to eq('Start later') + expect(sort_options[4]).to eq('Name, ascending') + expect(sort_options[5]).to eq('Name, descending') click_link 'Due later' @@ -40,6 +44,7 @@ feature 'Milestones sorting', :js do within '.milestones' do expect(page.all('ul.content-list > li').first.text).to include('v1.0') + expect(page.all('ul.content-list > li')[1].text).to include('v3.0') expect(page.all('ul.content-list > li').last.text).to include('v2.0') end end -- cgit v1.2.1 From fc8bbf3b49ccd0e082c594167797f0b8eedf14a1 Mon Sep 17 00:00:00 2001 From: Ben Bodenmiller Date: Tue, 7 Nov 2017 23:33:10 +0000 Subject: formatting cleanup, swamp details, unicorn defaults --- doc/install/requirements.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/install/requirements.md b/doc/install/requirements.md index 7bf126eec5d..baecf9455b0 100644 --- a/doc/install/requirements.md +++ b/doc/install/requirements.md @@ -80,13 +80,13 @@ errors during usage. - 256GB RAM supports up to 32,000 users - More users? Run it on [multiple application servers](https://about.gitlab.com/high-availability/) -We recommend having at least 2GB of swap on your server, even if you currently have +We recommend having at least [2GB of swap on your server](https://askubuntu.com/a/505344/310789), even if you currently have enough available RAM. Having swap will help reduce the chance of errors occurring if your available memory changes. We also recommend [configuring the kernel's swappiness setting](https://askubuntu.com/a/103916) to a low value like `10` to make the most of your RAM while still having the swap available when needed. -Notice: The 25 workers of Sidekiq will show up as separate processes in your process overview (such as top or htop) but they share the same RAM allocation since Sidekiq is a multithreaded application. Please see the section below about Unicorn workers for information about how many you need of those. +Notice: The 25 workers of Sidekiq will show up as separate processes in your process overview (such as `top` or `htop`) but they share the same RAM allocation since Sidekiq is a multithreaded application. Please see the section below about Unicorn workers for information about how many you need of those. ## Database @@ -146,7 +146,7 @@ So for a machine with 2 cores, 3 unicorn workers is ideal. For all machines that have 2GB and up we recommend a minimum of three unicorn workers. If you have a 1GB machine we recommend to configure only two Unicorn workers to prevent excessive swapping. -To change the Unicorn workers when you have the Omnibus package please see [the Unicorn settings in the Omnibus GitLab documentation](https://gitlab.com/gitlab-org/omnibus-gitlab/blob/master/doc/settings/unicorn.md#unicorn-settings). +To change the Unicorn workers when you have the Omnibus package (which defaults to the recommendation above) please see [the Unicorn settings in the Omnibus GitLab documentation](https://gitlab.com/gitlab-org/omnibus-gitlab/blob/master/doc/settings/unicorn.md#unicorn-settings). ## Redis and Sidekiq -- cgit v1.2.1 From ecb151658d23418b98267c0dd4d0850f881ea9a8 Mon Sep 17 00:00:00 2001 From: Mark Pundsack Date: Thu, 9 Nov 2017 11:43:39 -0600 Subject: Simplify Feature Proposal template --- .gitlab/issue_templates/Feature Proposal.md | 43 +---------------------------- 1 file changed, 1 insertion(+), 42 deletions(-) diff --git a/.gitlab/issue_templates/Feature Proposal.md b/.gitlab/issue_templates/Feature Proposal.md index 1278061a410..5b55eb1374b 100644 --- a/.gitlab/issue_templates/Feature Proposal.md +++ b/.gitlab/issue_templates/Feature Proposal.md @@ -1,22 +1,3 @@ -Please read this! - -Before opening a new issue, make sure to search for keywords in the issues -filtered by the "feature proposal" label: - -For the Community Edition issue tracker: - -- https://gitlab.com/gitlab-org/gitlab-ce/issues?label_name%5B%5D=feature+proposal - -For the Enterprise Edition issue tracker: - -- https://gitlab.com/gitlab-org/gitlab-ee/issues?label_name%5B%5D=feature+proposal - -and verify the issue you're about to submit isn't a duplicate. - -Please remove this notice if you're confident your issue isn't a duplicate. - ------- - ### Description (Include problem, use cases, benefits, and/or goals) @@ -25,26 +6,4 @@ Please remove this notice if you're confident your issue isn't a duplicate. ### Links / references -### Documentation blurb - -#### Overview - -What is it? -Why should someone use this feature? -What is the underlying (business) problem? -How do you use this feature? - -#### Use cases - -Who is this for? Provide one or more use cases. - -### Feature checklist - -Make sure these are completed before closing the issue, -with a link to the relevant commit. - -- [ ] [Feature assurance](https://about.gitlab.com/handbook/product/#feature-assurance) -- [ ] Documentation -- [ ] Added to [features.yml](https://gitlab.com/gitlab-com/www-gitlab-com/blob/master/data/features.yml) - -/label ~"feature proposal" \ No newline at end of file +/label ~"feature proposal" -- cgit v1.2.1 From 74b87f02db2ebda0b2b16a60dd6759fe6e8de95a Mon Sep 17 00:00:00 2001 From: Clement Ho Date: Fri, 10 Nov 2017 17:41:04 -0600 Subject: Backport of add-epic-sidebar --- .../javascripts/lib/utils/datetime_utility.js | 15 +- app/assets/javascripts/lib/utils/text_utility.js | 4 + .../javascripts/vue_shared/components/pikaday.vue | 79 ++++++++++ .../components/sidebar/collapsed_calendar_icon.vue | 46 ++++++ .../sidebar/collapsed_grouped_date_picker.vue | 109 ++++++++++++++ .../vue_shared/components/sidebar/date_picker.vue | 163 +++++++++++++++++++++ .../components/sidebar/toggle_sidebar.vue | 30 ++++ app/assets/stylesheets/framework/buttons.scss | 23 +++ app/assets/stylesheets/framework/sidebar.scss | 25 +++- app/assets/stylesheets/pages/issuable.scss | 29 +++- spec/javascripts/datetime_utility_spec.js | 22 ++- spec/javascripts/lib/utils/text_utility_spec.js | 14 +- .../vue_shared/components/pikaday_spec.js | 29 ++++ .../sidebar/collapsed_calendar_icon_spec.js | 35 +++++ .../sidebar/collapsed_grouped_date_picker_spec.js | 91 ++++++++++++ .../components/sidebar/date_picker_spec.js | 117 +++++++++++++++ .../components/sidebar/toggle_sidebar_spec.js | 32 ++++ 17 files changed, 849 insertions(+), 14 deletions(-) create mode 100644 app/assets/javascripts/vue_shared/components/pikaday.vue create mode 100644 app/assets/javascripts/vue_shared/components/sidebar/collapsed_calendar_icon.vue create mode 100644 app/assets/javascripts/vue_shared/components/sidebar/collapsed_grouped_date_picker.vue create mode 100644 app/assets/javascripts/vue_shared/components/sidebar/date_picker.vue create mode 100644 app/assets/javascripts/vue_shared/components/sidebar/toggle_sidebar.vue create mode 100644 spec/javascripts/vue_shared/components/pikaday_spec.js create mode 100644 spec/javascripts/vue_shared/components/sidebar/collapsed_calendar_icon_spec.js create mode 100644 spec/javascripts/vue_shared/components/sidebar/collapsed_grouped_date_picker_spec.js create mode 100644 spec/javascripts/vue_shared/components/sidebar/date_picker_spec.js create mode 100644 spec/javascripts/vue_shared/components/sidebar/toggle_sidebar_spec.js diff --git a/app/assets/javascripts/lib/utils/datetime_utility.js b/app/assets/javascripts/lib/utils/datetime_utility.js index 29fc91733b3..8d0bab55843 100644 --- a/app/assets/javascripts/lib/utils/datetime_utility.js +++ b/app/assets/javascripts/lib/utils/datetime_utility.js @@ -135,7 +135,6 @@ window.dateFormat = dateFormat; * @param {Number} seconds * @return {String} */ -// eslint-disable-next-line import/prefer-default-export export function timeIntervalInWords(intervalInSeconds) { const secondsInteger = parseInt(intervalInSeconds, 10); const minutes = Math.floor(secondsInteger / 60); @@ -149,3 +148,17 @@ export function timeIntervalInWords(intervalInSeconds) { } return text; } + +export function dateInWords(date, abbreviated = false) { + if (!date) return date; + + const month = date.getMonth(); + const year = date.getFullYear(); + + const monthNames = [s__('January'), s__('February'), s__('March'), s__('April'), s__('May'), s__('June'), s__('July'), s__('August'), s__('September'), s__('October'), s__('November'), s__('December')]; + const monthNamesAbbr = [s__('Jan'), s__('Feb'), s__('Mar'), s__('Apr'), s__('May'), s__('Jun'), s__('Jul'), s__('Aug'), s__('Sep'), s__('Oct'), s__('Nov'), s__('Dec')]; + + const monthName = abbreviated ? monthNamesAbbr[month] : monthNames[month]; + + return `${monthName} ${date.getDate()}, ${year}`; +} diff --git a/app/assets/javascripts/lib/utils/text_utility.js b/app/assets/javascripts/lib/utils/text_utility.js index f776829f69c..0decfff6921 100644 --- a/app/assets/javascripts/lib/utils/text_utility.js +++ b/app/assets/javascripts/lib/utils/text_utility.js @@ -24,6 +24,10 @@ export function highCountTrim(count) { return count > 99 ? '99+' : count; } +export function capitalizeFirstCharacter(text) { + return `${text[0].toUpperCase()}${text.slice(1)}`; +} + gl.text.randomString = function() { return Math.random().toString(36).substring(7); }; diff --git a/app/assets/javascripts/vue_shared/components/pikaday.vue b/app/assets/javascripts/vue_shared/components/pikaday.vue new file mode 100644 index 00000000000..d8d974a2ff7 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/pikaday.vue @@ -0,0 +1,79 @@ + + + diff --git a/app/assets/javascripts/vue_shared/components/sidebar/collapsed_calendar_icon.vue b/app/assets/javascripts/vue_shared/components/sidebar/collapsed_calendar_icon.vue new file mode 100644 index 00000000000..a88e1310131 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/sidebar/collapsed_calendar_icon.vue @@ -0,0 +1,46 @@ + + + diff --git a/app/assets/javascripts/vue_shared/components/sidebar/collapsed_grouped_date_picker.vue b/app/assets/javascripts/vue_shared/components/sidebar/collapsed_grouped_date_picker.vue new file mode 100644 index 00000000000..9ede5553bc5 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/sidebar/collapsed_grouped_date_picker.vue @@ -0,0 +1,109 @@ + + + diff --git a/app/assets/javascripts/vue_shared/components/sidebar/date_picker.vue b/app/assets/javascripts/vue_shared/components/sidebar/date_picker.vue new file mode 100644 index 00000000000..9c3413377a3 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/sidebar/date_picker.vue @@ -0,0 +1,163 @@ + + + diff --git a/app/assets/javascripts/vue_shared/components/sidebar/toggle_sidebar.vue b/app/assets/javascripts/vue_shared/components/sidebar/toggle_sidebar.vue new file mode 100644 index 00000000000..5ae76adad71 --- /dev/null +++ b/app/assets/javascripts/vue_shared/components/sidebar/toggle_sidebar.vue @@ -0,0 +1,30 @@ + + + diff --git a/app/assets/stylesheets/framework/buttons.scss b/app/assets/stylesheets/framework/buttons.scss index b2f26cf7159..dfa3d4c6fb9 100644 --- a/app/assets/stylesheets/framework/buttons.scss +++ b/app/assets/stylesheets/framework/buttons.scss @@ -408,6 +408,7 @@ padding: 0; background: transparent; border: 0; + border-radius: 0; &:hover, &:active, @@ -417,3 +418,25 @@ box-shadow: none; } } + +.btn-link.btn-secondary-hover-link { + color: $gl-text-color-secondary; + + &:hover, + &:active, + &:focus { + color: $gl-link-color; + text-decoration: none; + } +} + +.btn-link.btn-primary-hover-link { + color: inherit; + + &:hover, + &:active, + &:focus { + color: $gl-link-color; + text-decoration: none; + } +} diff --git a/app/assets/stylesheets/framework/sidebar.scss b/app/assets/stylesheets/framework/sidebar.scss index 1a19b7320a0..792981fdc48 100644 --- a/app/assets/stylesheets/framework/sidebar.scss +++ b/app/assets/stylesheets/framework/sidebar.scss @@ -43,11 +43,13 @@ } .sidebar-collapsed-icon { - cursor: pointer; - .btn { background-color: $gray-light; } + + &:not(.disabled) { + cursor: pointer; + } } } @@ -55,6 +57,10 @@ padding-right: 0; z-index: 300; + .btn-sidebar-action { + display: inline-flex; + } + @media (min-width: $screen-sm-min) and (max-width: $screen-sm-max) { &:not(.wiki-sidebar):not(.build-sidebar):not(.issuable-bulk-update-sidebar) .content-wrapper { padding-right: $gutter_collapsed_width; @@ -136,3 +142,18 @@ .issuable-sidebar { @include new-style-dropdown; } + +.pikaday-container { + .pika-single { + margin-top: 2px; + width: 250px; + } + + .dropdown-menu-toggle { + line-height: 20px; + } +} + +.sidebar-collapsed-icon .sidebar-collapsed-value { + font-size: 12px; +} diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index 760c7c80aff..d9d00197dc0 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -284,10 +284,15 @@ font-weight: $gl-font-weight-normal; } - .no-value { + .no-value, + .btn-secondary-hover-link { color: $gl-text-color-secondary; } + .btn-secondary-hover-link:hover { + color: $gl-link-color; + } + .sidebar-collapsed-icon { display: none; } @@ -295,6 +300,8 @@ .gutter-toggle { margin-top: 7px; border-left: 1px solid $border-gray-normal; + padding-left: 0; + text-align: center; } .title .gutter-toggle { @@ -367,7 +374,7 @@ fill: $issuable-sidebar-color; } - &:hover, + &:hover:not(.disabled), &:hover .todo-undone { color: $gl-text-color; @@ -908,3 +915,21 @@ margin: 0 3px; } } + +.right-sidebar-collapsed { + .sidebar-grouped-item { + .sidebar-collapsed-icon { + margin-bottom: 0; + } + + .sidebar-collapsed-divider { + line-height: 5px; + font-size: 12px; + color: $theme-gray-700; + + + .sidebar-collapsed-icon { + padding-top: 0; + } + } + } +} diff --git a/spec/javascripts/datetime_utility_spec.js b/spec/javascripts/datetime_utility_spec.js index 3391cade541..0f7bf9ec712 100644 --- a/spec/javascripts/datetime_utility_spec.js +++ b/spec/javascripts/datetime_utility_spec.js @@ -1,4 +1,4 @@ -import { timeIntervalInWords } from '~/lib/utils/datetime_utility'; +import * as datetimeUtility from '~/lib/utils/datetime_utility'; (() => { describe('Date time utils', () => { @@ -89,10 +89,22 @@ import { timeIntervalInWords } from '~/lib/utils/datetime_utility'; describe('timeIntervalInWords', () => { it('should return string with number of minutes and seconds', () => { - expect(timeIntervalInWords(9.54)).toEqual('9 seconds'); - expect(timeIntervalInWords(1)).toEqual('1 second'); - expect(timeIntervalInWords(200)).toEqual('3 minutes 20 seconds'); - expect(timeIntervalInWords(6008)).toEqual('100 minutes 8 seconds'); + expect(datetimeUtility.timeIntervalInWords(9.54)).toEqual('9 seconds'); + expect(datetimeUtility.timeIntervalInWords(1)).toEqual('1 second'); + expect(datetimeUtility.timeIntervalInWords(200)).toEqual('3 minutes 20 seconds'); + expect(datetimeUtility.timeIntervalInWords(6008)).toEqual('100 minutes 8 seconds'); + }); + }); + + describe('dateInWords', () => { + const date = new Date('07/01/2016'); + + it('should return date in words', () => { + expect(datetimeUtility.dateInWords(date)).toEqual('July 1, 2016'); + }); + + it('should return abbreviated month name', () => { + expect(datetimeUtility.dateInWords(date, true)).toEqual('Jul 1, 2016'); }); }); })(); diff --git a/spec/javascripts/lib/utils/text_utility_spec.js b/spec/javascripts/lib/utils/text_utility_spec.js index 829b3ef5735..f2e2ce79d27 100644 --- a/spec/javascripts/lib/utils/text_utility_spec.js +++ b/spec/javascripts/lib/utils/text_utility_spec.js @@ -1,4 +1,4 @@ -import { highCountTrim } from '~/lib/utils/text_utility'; +import * as textUtility from '~/lib/utils/text_utility'; describe('text_utility', () => { describe('gl.text.getTextWidth', () => { @@ -37,12 +37,18 @@ describe('text_utility', () => { describe('highCountTrim', () => { it('returns 99+ for count >= 100', () => { - expect(highCountTrim(105)).toBe('99+'); - expect(highCountTrim(100)).toBe('99+'); + expect(textUtility.highCountTrim(105)).toBe('99+'); + expect(textUtility.highCountTrim(100)).toBe('99+'); }); it('returns exact number for count < 100', () => { - expect(highCountTrim(45)).toBe(45); + expect(textUtility.highCountTrim(45)).toBe(45); + }); + }); + + describe('capitalizeFirstCharacter', () => { + it('returns string with first letter capitalized', () => { + expect(textUtility.capitalizeFirstCharacter('gitlab')).toEqual('Gitlab'); }); }); diff --git a/spec/javascripts/vue_shared/components/pikaday_spec.js b/spec/javascripts/vue_shared/components/pikaday_spec.js new file mode 100644 index 00000000000..47af9534737 --- /dev/null +++ b/spec/javascripts/vue_shared/components/pikaday_spec.js @@ -0,0 +1,29 @@ +import Vue from 'vue'; +import datePicker from '~/vue_shared/components/pikaday.vue'; +import mountComponent from '../../helpers/vue_mount_component_helper'; + +describe('datePicker', () => { + let vm; + beforeEach(() => { + const DatePicker = Vue.extend(datePicker); + vm = mountComponent(DatePicker, { + label: 'label', + }); + }); + + it('should render label text', () => { + expect(vm.$el.querySelector('.dropdown-toggle-text').innerText.trim()).toEqual('label'); + }); + + it('should show calendar', () => { + expect(vm.$el.querySelector('.pika-single')).toBeDefined(); + }); + + it('should toggle when dropdown is clicked', () => { + const hidePicker = jasmine.createSpy(); + vm.$on('hidePicker', hidePicker); + + vm.$el.querySelector('.dropdown-menu-toggle').click(); + expect(hidePicker).toHaveBeenCalled(); + }); +}); diff --git a/spec/javascripts/vue_shared/components/sidebar/collapsed_calendar_icon_spec.js b/spec/javascripts/vue_shared/components/sidebar/collapsed_calendar_icon_spec.js new file mode 100644 index 00000000000..cce53193870 --- /dev/null +++ b/spec/javascripts/vue_shared/components/sidebar/collapsed_calendar_icon_spec.js @@ -0,0 +1,35 @@ +import Vue from 'vue'; +import collapsedCalendarIcon from '~/vue_shared/components/sidebar/collapsed_calendar_icon.vue'; +import mountComponent from '../../../helpers/vue_mount_component_helper'; + +describe('collapsedCalendarIcon', () => { + let vm; + beforeEach(() => { + const CollapsedCalendarIcon = Vue.extend(collapsedCalendarIcon); + vm = mountComponent(CollapsedCalendarIcon, { + containerClass: 'test-class', + text: 'text', + showIcon: false, + }); + }); + + it('should add class to container', () => { + expect(vm.$el.classList.contains('test-class')).toEqual(true); + }); + + it('should hide calendar icon if showIcon', () => { + expect(vm.$el.querySelector('.fa-calendar')).toBeNull(); + }); + + it('should render text', () => { + expect(vm.$el.querySelector('span').innerText.trim()).toEqual('text'); + }); + + it('should emit click event when container is clicked', () => { + const click = jasmine.createSpy(); + vm.$on('click', click); + + vm.$el.click(); + expect(click).toHaveBeenCalled(); + }); +}); diff --git a/spec/javascripts/vue_shared/components/sidebar/collapsed_grouped_date_picker_spec.js b/spec/javascripts/vue_shared/components/sidebar/collapsed_grouped_date_picker_spec.js new file mode 100644 index 00000000000..20363e78094 --- /dev/null +++ b/spec/javascripts/vue_shared/components/sidebar/collapsed_grouped_date_picker_spec.js @@ -0,0 +1,91 @@ +import Vue from 'vue'; +import collapsedGroupedDatePicker from '~/vue_shared/components/sidebar/collapsed_grouped_date_picker.vue'; +import mountComponent from '../../../helpers/vue_mount_component_helper'; + +describe('collapsedGroupedDatePicker', () => { + let vm; + beforeEach(() => { + const CollapsedGroupedDatePicker = Vue.extend(collapsedGroupedDatePicker); + vm = mountComponent(CollapsedGroupedDatePicker, { + showToggleSidebar: true, + }); + }); + + it('should render toggle sidebar if showToggleSidebar', (done) => { + expect(vm.$el.querySelector('.issuable-sidebar-header')).toBeDefined(); + + vm.showToggleSidebar = false; + Vue.nextTick(() => { + expect(vm.$el.querySelector('.issuable-sidebar-header')).toBeNull(); + done(); + }); + }); + + it('toggleCollapse events', () => { + const toggleCollapse = jasmine.createSpy(); + + beforeEach((done) => { + vm.minDate = new Date('07/17/2016'); + Vue.nextTick(done); + }); + + it('should emit when sidebar is toggled', () => { + vm.$el.querySelector('.gutter-toggle').click(); + expect(toggleCollapse).toHaveBeenCalled(); + }); + + it('should emit when collapsed-calendar-icon is clicked', () => { + vm.$el.querySelector('.sidebar-collapsed-icon').click(); + expect(toggleCollapse).toHaveBeenCalled(); + }); + }); + + describe('minDate and maxDate', () => { + beforeEach((done) => { + vm.minDate = new Date('07/17/2016'); + vm.maxDate = new Date('07/17/2017'); + Vue.nextTick(done); + }); + + it('should render both collapsed-calendar-icon', () => { + const icons = vm.$el.querySelectorAll('.sidebar-collapsed-icon'); + expect(icons.length).toEqual(2); + expect(icons[0].innerText.trim()).toEqual('Jul 17 2016'); + expect(icons[1].innerText.trim()).toEqual('Jul 17 2017'); + }); + }); + + describe('minDate', () => { + beforeEach((done) => { + vm.minDate = new Date('07/17/2016'); + Vue.nextTick(done); + }); + + it('should render minDate in collapsed-calendar-icon', () => { + const icons = vm.$el.querySelectorAll('.sidebar-collapsed-icon'); + expect(icons.length).toEqual(1); + expect(icons[0].innerText.trim()).toEqual('From Jul 17 2016'); + }); + }); + + describe('maxDate', () => { + beforeEach((done) => { + vm.maxDate = new Date('07/17/2017'); + Vue.nextTick(done); + }); + + it('should render maxDate in collapsed-calendar-icon', () => { + const icons = vm.$el.querySelectorAll('.sidebar-collapsed-icon'); + expect(icons.length).toEqual(1); + expect(icons[0].innerText.trim()).toEqual('Until Jul 17 2017'); + }); + }); + + describe('no dates', () => { + it('should render None', () => { + const icons = vm.$el.querySelectorAll('.sidebar-collapsed-icon'); + expect(icons.length).toEqual(1); + expect(icons[0].innerText.trim()).toEqual('None'); + }); + }); +}); diff --git a/spec/javascripts/vue_shared/components/sidebar/date_picker_spec.js b/spec/javascripts/vue_shared/components/sidebar/date_picker_spec.js new file mode 100644 index 00000000000..926e11b4d30 --- /dev/null +++ b/spec/javascripts/vue_shared/components/sidebar/date_picker_spec.js @@ -0,0 +1,117 @@ +import Vue from 'vue'; +import sidebarDatePicker from '~/vue_shared/components/sidebar/date_picker.vue'; +import mountComponent from '../../../helpers/vue_mount_component_helper'; + +describe('sidebarDatePicker', () => { + let vm; + beforeEach(() => { + const SidebarDatePicker = Vue.extend(sidebarDatePicker); + vm = mountComponent(SidebarDatePicker, { + label: 'label', + isLoading: true, + }); + }); + + it('should emit toggleCollapse when collapsed toggle sidebar is clicked', () => { + const toggleCollapse = jasmine.createSpy(); + vm.$on('toggleCollapse', toggleCollapse); + + vm.$el.querySelector('.issuable-sidebar-header .gutter-toggle').click(); + expect(toggleCollapse).toHaveBeenCalled(); + }); + + it('should render collapsed-calendar-icon', () => { + expect(vm.$el.querySelector('.sidebar-collapsed-icon')).toBeDefined(); + }); + + it('should render label', () => { + expect(vm.$el.querySelector('.title').innerText.trim()).toEqual('label'); + }); + + it('should render loading-icon when isLoading', () => { + expect(vm.$el.querySelector('.fa-spin')).toBeDefined(); + }); + + it('should render value when not editing', () => { + expect(vm.$el.querySelector('.value-content')).toBeDefined(); + }); + + it('should render None if there is no selectedDate', () => { + expect(vm.$el.querySelector('.value-content span').innerText.trim()).toEqual('None'); + }); + + it('should render date-picker when editing', (done) => { + vm.editing = true; + Vue.nextTick(() => { + expect(vm.$el.querySelector('.pika-label')).toBeDefined(); + done(); + }); + }); + + describe('editable', () => { + beforeEach((done) => { + vm.editable = true; + Vue.nextTick(done); + }); + + it('should render edit button', () => { + expect(vm.$el.querySelector('.title .btn-blank').innerText.trim()).toEqual('Edit'); + }); + + it('should enable editing when edit button is clicked', (done) => { + vm.isLoading = false; + Vue.nextTick(() => { + vm.$el.querySelector('.title .btn-blank').click(); + expect(vm.editing).toEqual(true); + done(); + }); + }); + }); + + it('should render date if selectedDate', (done) => { + vm.selectedDate = new Date('07/07/2017'); + Vue.nextTick(() => { + expect(vm.$el.querySelector('.value-content strong').innerText.trim()).toEqual('Jul 7, 2017'); + done(); + }); + }); + + describe('selectedDate and editable', () => { + beforeEach((done) => { + vm.selectedDate = new Date('07/07/2017'); + vm.editable = true; + Vue.nextTick(done); + }); + + it('should render remove button if selectedDate and editable', () => { + expect(vm.$el.querySelector('.value-content .btn-blank').innerText.trim()).toEqual('remove'); + }); + + it('should emit saveDate when remove button is clicked', () => { + const saveDate = jasmine.createSpy(); + vm.$on('saveDate', saveDate); + + vm.$el.querySelector('.value-content .btn-blank').click(); + expect(saveDate).toHaveBeenCalled(); + }); + }); + + describe('showToggleSidebar', () => { + beforeEach((done) => { + vm.showToggleSidebar = true; + Vue.nextTick(done); + }); + + it('should render toggle-sidebar when showToggleSidebar', () => { + expect(vm.$el.querySelector('.title .gutter-toggle')).toBeDefined(); + }); + + it('should emit toggleCollapse when toggle sidebar is clicked', () => { + const toggleCollapse = jasmine.createSpy(); + vm.$on('toggleCollapse', toggleCollapse); + + vm.$el.querySelector('.title .gutter-toggle').click(); + expect(toggleCollapse).toHaveBeenCalled(); + }); + }); +}); diff --git a/spec/javascripts/vue_shared/components/sidebar/toggle_sidebar_spec.js b/spec/javascripts/vue_shared/components/sidebar/toggle_sidebar_spec.js new file mode 100644 index 00000000000..752a9e89d50 --- /dev/null +++ b/spec/javascripts/vue_shared/components/sidebar/toggle_sidebar_spec.js @@ -0,0 +1,32 @@ +import Vue from 'vue'; +import toggleSidebar from '~/vue_shared/components/sidebar/toggle_sidebar.vue'; +import mountComponent from '../../../helpers/vue_mount_component_helper'; + +describe('toggleSidebar', () => { + let vm; + beforeEach(() => { + const ToggleSidebar = Vue.extend(toggleSidebar); + vm = mountComponent(ToggleSidebar, { + collapsed: true, + }); + }); + + it('should render << when collapsed', () => { + expect(vm.$el.querySelector('.fa').classList.contains('fa-angle-double-left')).toEqual(true); + }); + + it('should render >> when collapsed', () => { + vm.collapsed = false; + Vue.nextTick(() => { + expect(vm.$el.querySelector('.fa').classList.contains('fa-angle-double-right')).toEqual(true); + }); + }); + + it('should emit toggle event when button clicked', () => { + const toggle = jasmine.createSpy(); + vm.$on('toggle', toggle); + vm.$el.click(); + + expect(toggle).toHaveBeenCalled(); + }); +}); -- cgit v1.2.1 From 6ae4747673aacce567ad17b16a6f957e038452c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 14 Nov 2017 11:06:57 +0100 Subject: Replace ci_status_path with pipeline_path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/helpers/ci_status_helper.rb | 5 ----- app/views/admin/runners/show.html.haml | 2 +- spec/features/commits_spec.rb | 22 ++++++++++------------ spec/javascripts/vue_mr_widget/mock_data.js | 1 - 4 files changed, 11 insertions(+), 19 deletions(-) diff --git a/app/helpers/ci_status_helper.rb b/app/helpers/ci_status_helper.rb index 4dd573c61f1..636316da80a 100644 --- a/app/helpers/ci_status_helper.rb +++ b/app/helpers/ci_status_helper.rb @@ -6,11 +6,6 @@ # See 'detailed_status?` method and `Gitlab::Ci::Status` module. # module CiStatusHelper - def ci_status_path(pipeline) - project = pipeline.project - project_pipeline_path(project, pipeline) - end - def ci_label_for_status(status) if detailed_status?(status) return status.label diff --git a/app/views/admin/runners/show.html.haml b/app/views/admin/runners/show.html.haml index df2bf27be9d..6d8fad0eb8d 100644 --- a/app/views/admin/runners/show.html.haml +++ b/app/views/admin/runners/show.html.haml @@ -99,7 +99,7 @@ %td.build-link - if project - = link_to ci_status_path(build.pipeline) do + = link_to pipeline_path(build.pipeline) do %strong= build.pipeline.short_sha %td.timestamp diff --git a/spec/features/commits_spec.rb b/spec/features/commits_spec.rb index 479fb713297..b163ca8dc75 100644 --- a/spec/features/commits_spec.rb +++ b/spec/features/commits_spec.rb @@ -1,8 +1,6 @@ require 'spec_helper' describe 'Commits' do - include CiStatusHelper - let(:project) { create(:project, :repository) } let(:user) { create(:user) } @@ -33,7 +31,7 @@ describe 'Commits' do describe 'Commit builds' do before do - visit ci_status_path(pipeline) + visit pipeline_path(pipeline) end it { expect(page).to have_content pipeline.sha[0..7] } @@ -79,7 +77,7 @@ describe 'Commits' do describe 'Commit builds', :js do before do - visit ci_status_path(pipeline) + visit pipeline_path(pipeline) end it 'shows pipeline`s data' do @@ -95,7 +93,7 @@ describe 'Commits' do end it do - visit ci_status_path(pipeline) + visit pipeline_path(pipeline) click_on 'Download artifacts' expect(page.response_headers['Content-Type']).to eq(artifacts_file.content_type) end @@ -103,7 +101,7 @@ describe 'Commits' do describe 'Cancel all builds' do it 'cancels commit', :js do - visit ci_status_path(pipeline) + visit pipeline_path(pipeline) click_on 'Cancel running' expect(page).to have_content 'canceled' end @@ -111,7 +109,7 @@ describe 'Commits' do describe 'Cancel build' do it 'cancels build', :js do - visit ci_status_path(pipeline) + visit pipeline_path(pipeline) find('.js-btn-cancel-pipeline').click expect(page).to have_content 'canceled' end @@ -120,13 +118,13 @@ describe 'Commits' do describe '.gitlab-ci.yml not found warning' do context 'ci builds enabled' do it "does not show warning" do - visit ci_status_path(pipeline) + visit pipeline_path(pipeline) expect(page).not_to have_content '.gitlab-ci.yml not found in this commit' end it 'shows warning' do stub_ci_pipeline_yaml_file(nil) - visit ci_status_path(pipeline) + visit pipeline_path(pipeline) expect(page).to have_content '.gitlab-ci.yml not found in this commit' end end @@ -135,7 +133,7 @@ describe 'Commits' do before do stub_ci_builds_disabled stub_ci_pipeline_yaml_file(nil) - visit ci_status_path(pipeline) + visit pipeline_path(pipeline) end it 'does not show warning' do @@ -149,7 +147,7 @@ describe 'Commits' do before do project.team << [user, :reporter] build.update_attributes(artifacts_file: artifacts_file) - visit ci_status_path(pipeline) + visit pipeline_path(pipeline) end it 'Renders header', :js do @@ -171,7 +169,7 @@ describe 'Commits' do visibility_level: Gitlab::VisibilityLevel::INTERNAL, public_builds: false) build.update_attributes(artifacts_file: artifacts_file) - visit ci_status_path(pipeline) + visit pipeline_path(pipeline) end it do diff --git a/spec/javascripts/vue_mr_widget/mock_data.js b/spec/javascripts/vue_mr_widget/mock_data.js index 0795d0aaa82..1ad7c2d8efa 100644 --- a/spec/javascripts/vue_mr_widget/mock_data.js +++ b/spec/javascripts/vue_mr_widget/mock_data.js @@ -202,7 +202,6 @@ export default { "revert_in_fork_path": "/root/acets-app/forks?continue%5Bnotice%5D=You%27re+not+allowed+to+make+changes+to+this+project+directly.+A+fork+of+this+project+has+been+created+that+you+can+make+changes+in%2C+so+you+can+submit+a+merge+request.+Try+to+cherry-pick+this+commit+again.&continue%5Bnotice_now%5D=You%27re+not+allowed+to+make+changes+to+this+project+directly.+A+fork+of+this+project+is+being+created+that+you+can+make+changes+in%2C+so+you+can+submit+a+merge+request.&continue%5Bto%5D=%2Froot%2Facets-app%2Fmerge_requests%2F22&namespace_key=1", "email_patches_path": "/root/acets-app/merge_requests/22.patch", "plain_diff_path": "/root/acets-app/merge_requests/22.diff", - "ci_status_path": "/root/acets-app/merge_requests/22/ci_status", "status_path": "/root/acets-app/merge_requests/22.json", "merge_check_path": "/root/acets-app/merge_requests/22/merge_check", "ci_environments_status_url": "/root/acets-app/merge_requests/22/ci_environments_status", -- cgit v1.2.1 From f961744bdea16159f22d9febf463a9f82157eafe Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Wed, 15 Nov 2017 10:13:57 +0100 Subject: Isolate the fork network background migrations Before the `PopulateForkNetworksRange` spec would also call the `CreateForkNetworkMemberships` which we would count on in the spec. With this, I'm isolating that, and counting only records created in this particular migration instead. --- .../populate_fork_networks_range_spec.rb | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/spec/lib/gitlab/background_migration/populate_fork_networks_range_spec.rb b/spec/lib/gitlab/background_migration/populate_fork_networks_range_spec.rb index 2c2684a6fc9..994992f79d4 100644 --- a/spec/lib/gitlab/background_migration/populate_fork_networks_range_spec.rb +++ b/spec/lib/gitlab/background_migration/populate_fork_networks_range_spec.rb @@ -3,12 +3,9 @@ require 'spec_helper' describe Gitlab::BackgroundMigration::PopulateForkNetworksRange, :migration, schema: 20170929131201 do let(:migration) { described_class.new } let(:base1) { create(:project) } - let(:base1_fork1) { create(:project) } - let(:base1_fork2) { create(:project) } let(:base2) { create(:project) } let(:base2_fork1) { create(:project) } - let(:base2_fork2) { create(:project) } let!(:forked_project_links) { table(:forked_project_links) } let!(:fork_networks) { table(:fork_networks) } @@ -21,21 +18,24 @@ describe Gitlab::BackgroundMigration::PopulateForkNetworksRange, :migration, sch # A normal fork link forked_project_links.create(id: 1, forked_from_project_id: base1.id, - forked_to_project_id: base1_fork1.id) + forked_to_project_id: create(:project).id) forked_project_links.create(id: 2, forked_from_project_id: base1.id, - forked_to_project_id: base1_fork2.id) - + forked_to_project_id: create(:project).id) forked_project_links.create(id: 3, forked_from_project_id: base2.id, forked_to_project_id: base2_fork1.id) + + # create a fork of a fork forked_project_links.create(id: 4, forked_from_project_id: base2_fork1.id, forked_to_project_id: create(:project).id) - forked_project_links.create(id: 5, - forked_from_project_id: base2.id, - forked_to_project_id: base2_fork2.id) + forked_from_project_id: create(:project).id, + forked_to_project_id: create(:project).id) + + # Stub out the calls to the other migrations + allow(BackgroundMigrationWorker).to receive(:perform_in) migration.perform(1, 3) end @@ -80,11 +80,11 @@ describe Gitlab::BackgroundMigration::PopulateForkNetworksRange, :migration, sch end it 'only processes a single batch of links at a time' do - expect(fork_network_members.count).to eq(5) + expect(fork_networks.count).to eq(2) migration.perform(3, 5) - expect(fork_network_members.count).to eq(7) + expect(fork_networks.count).to eq(3) end it 'can be repeated without effect' do -- cgit v1.2.1 From 4374a38f0e8fbd856458a60f47083fb4d084d695 Mon Sep 17 00:00:00 2001 From: "micael.bergeron" Date: Wed, 15 Nov 2017 09:12:04 -0500 Subject: fix the commit reference pattern from matching other entities --- app/models/commit.rb | 2 +- ...-no-longer-returns-label-additions-removals.yml | 5 +++++ spec/models/commit_spec.rb | 23 ++++++++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/39461-notes-api-for-issues-no-longer-returns-label-additions-removals.yml diff --git a/app/models/commit.rb b/app/models/commit.rb index 6dba154a6ea..e28bf28fab3 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -26,7 +26,7 @@ class Commit DIFF_HARD_LIMIT_LINES = 50000 MIN_SHA_LENGTH = 7 - COMMIT_SHA_PATTERN = /\h{#{MIN_SHA_LENGTH},40}/.freeze + COMMIT_SHA_PATTERN = /\b(? Date: Wed, 15 Nov 2017 10:56:17 -0500 Subject: try to fix the failing spec on external refs --- app/models/commit.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/commit.rb b/app/models/commit.rb index e28bf28fab3..b5411b2bf75 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -26,7 +26,7 @@ class Commit DIFF_HARD_LIMIT_LINES = 50000 MIN_SHA_LENGTH = 7 - COMMIT_SHA_PATTERN = /\b(? Date: Wed, 15 Nov 2017 15:31:17 -0600 Subject: When a custom header logo is present, don't show GitLab type logo In the new navigation header, when a user uploads a custom header logo GitLab will still show the 'GitLab' type logo. This is often undesirable as the user may want to upload a logo with their own name present. This change will toggle the GitLab type logo if a custom header logo is present. --- app/helpers/appearances_helper.rb | 7 +++++++ app/views/admin/appearances/_form.html.haml | 2 +- app/views/layouts/header/_default.html.haml | 2 +- changelogs/unreleased/brand_header_change.yml | 5 +++++ 4 files changed, 14 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/brand_header_change.yml diff --git a/app/helpers/appearances_helper.rb b/app/helpers/appearances_helper.rb index 8ad94d3f723..df590cf47c8 100644 --- a/app/helpers/appearances_helper.rb +++ b/app/helpers/appearances_helper.rb @@ -30,4 +30,11 @@ module AppearancesHelper render 'shared/logo.svg' end end + + # Skip the 'GitLab' type logo when custom brand logo is set + def brand_header_logo_type + unless brand_item && brand_item.header_logo? + render 'shared/logo_type.svg' + end + end end diff --git a/app/views/admin/appearances/_form.html.haml b/app/views/admin/appearances/_form.html.haml index 935787d1a4a..4a2238fe277 100644 --- a/app/views/admin/appearances/_form.html.haml +++ b/app/views/admin/appearances/_form.html.haml @@ -43,7 +43,7 @@ = f.hidden_field :header_logo_cache = f.file_field :header_logo, class: "" .hint - Maximum file size is 1MB. Pages are optimized for a 72x72 px header logo + Maximum file size is 1MB. Pages are optimized for a 28px tall header logo .form-actions = f.submit 'Save', class: 'btn btn-save append-right-10' diff --git a/app/views/layouts/header/_default.html.haml b/app/views/layouts/header/_default.html.haml index 1eca412aff9..e2407f6a428 100644 --- a/app/views/layouts/header/_default.html.haml +++ b/app/views/layouts/header/_default.html.haml @@ -7,7 +7,7 @@ = link_to root_path, title: 'Dashboard', id: 'logo' do = brand_header_logo %span.logo-text.hidden-xs - = render 'shared/logo_type.svg' + = brand_header_logo_type - if current_user = render "layouts/nav/dashboard" diff --git a/changelogs/unreleased/brand_header_change.yml b/changelogs/unreleased/brand_header_change.yml new file mode 100644 index 00000000000..6ea6e8192a4 --- /dev/null +++ b/changelogs/unreleased/brand_header_change.yml @@ -0,0 +1,5 @@ +--- +title: When a custom header logo is present, don't show GitLab type logo +merge_request: +author: +type: changed -- cgit v1.2.1 From a37427f614625502089b7390cf570a1c8a1b3da9 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Thu, 16 Nov 2017 11:57:19 +0100 Subject: Adjust openid_connect_spec to use `raise_error` Using the `raise_error`-matcher instead of `throw_symbol` makes sure our after blocks get called in the test suite. --- spec/requests/openid_connect_spec.rb | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/spec/requests/openid_connect_spec.rb b/spec/requests/openid_connect_spec.rb index 0b1f8ce6f6d..1a5ad9b04e4 100644 --- a/spec/requests/openid_connect_spec.rb +++ b/spec/requests/openid_connect_spec.rb @@ -107,6 +107,15 @@ describe 'OpenID Connect requests' do end end + # These 2 calls shouldn't actually throw, they should be handled as an + # unauthorized request, so we should be able to check the response. + # + # This was not possible due to an issue with Warden: + # https://github.com/hassox/warden/pull/162 + # + # When the patch gets merged and we update Warden, these specs will need to + # updated to check the response instead of a raised exception. + # https://gitlab.com/gitlab-org/gitlab-ce/issues/40218 context 'when user is blocked' do it 'returns authentication error' do access_grant @@ -114,7 +123,7 @@ describe 'OpenID Connect requests' do expect do request_access_token - end.to throw_symbol :warden + end.to raise_error UncaughtThrowError end end @@ -125,7 +134,7 @@ describe 'OpenID Connect requests' do expect do request_access_token - end.to throw_symbol :warden + end.to raise_error UncaughtThrowError end end end -- cgit v1.2.1 From 3e561736b2eb4866b75c57c01769586f058a2f8d Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Wed, 15 Nov 2017 15:47:10 +0100 Subject: Cache the number of user SSH keys By caching the number of personal SSH keys we reduce the number of queries necessary on pages such as ProjectsController#show (which can end up querying this data multiple times). The cache is refreshed/flushed whenever an SSH key is added, removed, or when a user is removed. --- app/models/key.rb | 8 +++ app/models/user.rb | 9 ++- app/services/base_count_service.rb | 34 +++++++++ app/services/projects/count_service.rb | 25 +------ app/services/projects/forks_count_service.rb | 2 +- app/services/projects/open_issues_count_service.rb | 2 +- .../projects/open_merge_requests_count_service.rb | 2 +- app/services/users/keys_count_service.rb | 27 ++++++++ changelogs/unreleased/cache-user-keys-count.yml | 5 ++ spec/models/key_spec.rb | 23 +++++++ spec/models/user_spec.rb | 8 ++- spec/services/base_count_service_spec.rb | 80 ++++++++++++++++++++++ spec/services/users/keys_count_service_spec.rb | 66 ++++++++++++++++++ 13 files changed, 262 insertions(+), 29 deletions(-) create mode 100644 app/services/base_count_service.rb create mode 100644 app/services/users/keys_count_service.rb create mode 100644 changelogs/unreleased/cache-user-keys-count.yml create mode 100644 spec/services/base_count_service_spec.rb create mode 100644 spec/services/users/keys_count_service_spec.rb diff --git a/app/models/key.rb b/app/models/key.rb index f119b15c737..815fd1de909 100644 --- a/app/models/key.rb +++ b/app/models/key.rb @@ -27,8 +27,10 @@ class Key < ActiveRecord::Base after_commit :add_to_shell, on: :create after_create :post_create_hook + after_create :refresh_user_cache after_commit :remove_from_shell, on: :destroy after_destroy :post_destroy_hook + after_destroy :refresh_user_cache def key=(value) value&.delete!("\n\r") @@ -76,6 +78,12 @@ class Key < ActiveRecord::Base ) end + def refresh_user_cache + return unless user + + Users::KeysCountService.new(user).refresh_cache + end + def post_destroy_hook SystemHooksService.new.execute_hooks_for(self, :destroy) end diff --git a/app/models/user.rb b/app/models/user.rb index ea10e2854d6..be8112749bf 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -170,6 +170,7 @@ class User < ActiveRecord::Base after_save :ensure_namespace_correct after_update :username_changed_hook, if: :username_changed? after_destroy :post_destroy_hook + after_destroy :remove_key_cache after_commit :update_emails_with_primary_email, on: :update, if: -> { previous_changes.key?('email') } after_commit :update_invalid_gpg_signatures, on: :update, if: -> { previous_changes.key?('email') } @@ -624,7 +625,9 @@ class User < ActiveRecord::Base end def require_ssh_key? - keys.count == 0 && Gitlab::ProtocolAccess.allowed?('ssh') + count = Users::KeysCountService.new(self).count + + count.zero? && Gitlab::ProtocolAccess.allowed?('ssh') end def require_password_creation? @@ -886,6 +889,10 @@ class User < ActiveRecord::Base system_hook_service.execute_hooks_for(self, :destroy) end + def remove_key_cache + Users::KeysCountService.new(self).delete_cache + end + def delete_async(deleted_by:, params: {}) block if params[:hard_delete] DeleteUserWorker.perform_async(deleted_by.id, id, params) diff --git a/app/services/base_count_service.rb b/app/services/base_count_service.rb new file mode 100644 index 00000000000..99cc9a196e6 --- /dev/null +++ b/app/services/base_count_service.rb @@ -0,0 +1,34 @@ +# Base class for services that count a single resource such as the number of +# issues for a project. +class BaseCountService + def relation_for_count + raise( + NotImplementedError, + '"relation_for_count" must be implemented and return an ActiveRecord::Relation' + ) + end + + def count + Rails.cache.fetch(cache_key, raw: raw?) { uncached_count }.to_i + end + + def refresh_cache + Rails.cache.write(cache_key, uncached_count, raw: raw?) + end + + def uncached_count + relation_for_count.count + end + + def delete_cache + Rails.cache.delete(cache_key) + end + + def raw? + false + end + + def cache_key + raise NotImplementedError, 'cache_key must be implemented and return a String' + end +end diff --git a/app/services/projects/count_service.rb b/app/services/projects/count_service.rb index aa034315280..7e575b2d6f3 100644 --- a/app/services/projects/count_service.rb +++ b/app/services/projects/count_service.rb @@ -1,7 +1,7 @@ module Projects # Base class for the various service classes that count project data (e.g. # issues or forks). - class CountService + class CountService < BaseCountService # The version of the cache format. This should be bumped whenever the # underlying logic changes. This removes the need for explicitly flushing # all caches. @@ -11,29 +11,6 @@ module Projects @project = project end - def relation_for_count - raise( - NotImplementedError, - '"relation_for_count" must be implemented and return an ActiveRecord::Relation' - ) - end - - def count - Rails.cache.fetch(cache_key) { uncached_count } - end - - def refresh_cache - Rails.cache.write(cache_key, uncached_count) - end - - def uncached_count - relation_for_count.count - end - - def delete_cache - Rails.cache.delete(cache_key) - end - def cache_key_name raise( NotImplementedError, diff --git a/app/services/projects/forks_count_service.rb b/app/services/projects/forks_count_service.rb index 3a0fa84b868..d9bdf3a8ad7 100644 --- a/app/services/projects/forks_count_service.rb +++ b/app/services/projects/forks_count_service.rb @@ -1,6 +1,6 @@ module Projects # Service class for getting and caching the number of forks of a project. - class ForksCountService < CountService + class ForksCountService < Projects::CountService def relation_for_count @project.forks end diff --git a/app/services/projects/open_issues_count_service.rb b/app/services/projects/open_issues_count_service.rb index 3c0d186a73c..25de97325e2 100644 --- a/app/services/projects/open_issues_count_service.rb +++ b/app/services/projects/open_issues_count_service.rb @@ -1,7 +1,7 @@ module Projects # Service class for counting and caching the number of open issues of a # project. - class OpenIssuesCountService < CountService + class OpenIssuesCountService < Projects::CountService def relation_for_count # We don't include confidential issues in this number since this would # expose the number of confidential issues to non project members. diff --git a/app/services/projects/open_merge_requests_count_service.rb b/app/services/projects/open_merge_requests_count_service.rb index 2a90f78b90d..77e6448fd5e 100644 --- a/app/services/projects/open_merge_requests_count_service.rb +++ b/app/services/projects/open_merge_requests_count_service.rb @@ -1,7 +1,7 @@ module Projects # Service class for counting and caching the number of open merge requests of # a project. - class OpenMergeRequestsCountService < CountService + class OpenMergeRequestsCountService < Projects::CountService def relation_for_count @project.merge_requests.opened end diff --git a/app/services/users/keys_count_service.rb b/app/services/users/keys_count_service.rb new file mode 100644 index 00000000000..f82d27eded9 --- /dev/null +++ b/app/services/users/keys_count_service.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module Users + # Service class for getting the number of SSH keys that belong to a user. + class KeysCountService < BaseCountService + attr_reader :user + + # user - The User for which to get the number of SSH keys. + def initialize(user) + @user = user + end + + def relation_for_count + user.keys + end + + def raw? + # Since we're storing simple integers we don't need all of the additional + # Marshal data Rails includes by default. + true + end + + def cache_key + "users/key-count-service/#{user.id}" + end + end +end diff --git a/changelogs/unreleased/cache-user-keys-count.yml b/changelogs/unreleased/cache-user-keys-count.yml new file mode 100644 index 00000000000..181be95622c --- /dev/null +++ b/changelogs/unreleased/cache-user-keys-count.yml @@ -0,0 +1,5 @@ +--- +title: Cache the number of user SSH keys +merge_request: +author: +type: performance diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb index 81c2057e175..4cd9e3f4f1d 100644 --- a/spec/models/key_spec.rb +++ b/spec/models/key_spec.rb @@ -166,4 +166,27 @@ describe Key, :mailer do expect(key.public_key.key_text).to eq(valid_key) end end + + describe '#refresh_user_cache', :use_clean_rails_memory_store_caching do + context 'when the key belongs to a user' do + it 'refreshes the keys count cache for the user' do + expect_any_instance_of(Users::KeysCountService) + .to receive(:refresh_cache) + .and_call_original + + key = create(:personal_key) + + expect(Users::KeysCountService.new(key.user).count).to eq(1) + end + end + + context 'when the key does not belong to a user' do + it 'does nothing' do + expect_any_instance_of(Users::KeysCountService) + .not_to receive(:refresh_cache) + + create(:key) + end + end + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 88732962071..86647ddf6ce 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -828,7 +828,7 @@ describe User do end end - describe '#require_ssh_key?' do + describe '#require_ssh_key?', :use_clean_rails_memory_store_caching do protocol_and_expectation = { 'http' => false, 'ssh' => true, @@ -843,6 +843,12 @@ describe User do expect(user.require_ssh_key?).to eq(expected) end end + + it 'returns false when the user has 1 or more SSH keys' do + key = create(:personal_key) + + expect(key.user.require_ssh_key?).to eq(false) + end end end diff --git a/spec/services/base_count_service_spec.rb b/spec/services/base_count_service_spec.rb new file mode 100644 index 00000000000..5ec8ed0976d --- /dev/null +++ b/spec/services/base_count_service_spec.rb @@ -0,0 +1,80 @@ +require 'spec_helper' + +describe BaseCountService, :use_clean_rails_memory_store_caching do + let(:service) { described_class.new } + + describe '#relation_for_count' do + it 'raises NotImplementedError' do + expect { service.relation_for_count }.to raise_error(NotImplementedError) + end + end + + describe '#count' do + it 'returns the number of values' do + expect(service) + .to receive(:cache_key) + .and_return('foo') + + expect(service) + .to receive(:uncached_count) + .and_return(5) + + expect(service.count).to eq(5) + end + end + + describe '#uncached_count' do + it 'returns the uncached number of values' do + expect(service) + .to receive(:relation_for_count) + .and_return(double(:relation, count: 5)) + + expect(service.uncached_count).to eq(5) + end + end + + describe '#refresh_cache' do + it 'refreshes the cache' do + allow(service) + .to receive(:cache_key) + .and_return('foo') + + allow(service) + .to receive(:uncached_count) + .and_return(4) + + service.refresh_cache + + expect(Rails.cache.fetch(service.cache_key, raw: service.raw?)).to eq(4) + end + end + + describe '#delete_cache' do + it 'deletes the cache' do + allow(service) + .to receive(:cache_key) + .and_return('foo') + + allow(service) + .to receive(:uncached_count) + .and_return(4) + + service.refresh_cache + service.delete_cache + + expect(Rails.cache.fetch(service.cache_key, raw: service.raw?)).to be_nil + end + end + + describe '#raw?' do + it 'returns false' do + expect(service.raw?).to eq(false) + end + end + + describe '#cache_key' do + it 'raises NotImplementedError' do + expect { service.cache_key }.to raise_error(NotImplementedError) + end + end +end diff --git a/spec/services/users/keys_count_service_spec.rb b/spec/services/users/keys_count_service_spec.rb new file mode 100644 index 00000000000..a188cf86772 --- /dev/null +++ b/spec/services/users/keys_count_service_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Users::KeysCountService, :use_clean_rails_memory_store_caching do + let(:user) { create(:user) } + let(:service) { described_class.new(user) } + + describe '#count' do + before do + create(:personal_key, user: user) + end + + it 'returns the number of SSH keys as an Integer' do + expect(service.count).to eq(1) + end + + it 'caches the number of keys in Redis' do + service.delete_cache + + recorder = ActiveRecord::QueryRecorder.new do + 2.times { service.count } + end + + expect(recorder.count).to eq(1) + end + end + + describe '#refresh_cache' do + it 'refreshes the Redis cache' do + Rails.cache.write(service.cache_key, 10) + service.refresh_cache + + expect(Rails.cache.fetch(service.cache_key, raw: true)).to be_zero + end + end + + describe '#delete_cache' do + it 'removes the cache' do + service.count + service.delete_cache + + expect(Rails.cache.fetch(service.cache_key, raw: true)).to be_nil + end + end + + describe '#uncached_count' do + it 'returns the number of SSH keys' do + expect(service.uncached_count).to be_zero + end + + it 'does not cache the number of keys' do + recorder = ActiveRecord::QueryRecorder.new do + 2.times { service.uncached_count } + end + + expect(recorder.count).to be > 0 + end + end + + describe '#cache_key' do + it 'returns the cache key' do + expect(service.cache_key).to eq("users/key-count-service/#{user.id}") + end + end +end -- cgit v1.2.1 From f691010d5c66b543c05ed4d53d663986b05dc90f Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 16 Nov 2017 12:23:50 +0100 Subject: Make sure NotesActions#noteable returns a Noteable in the update action --- app/controllers/concerns/notes_actions.rb | 10 +++++++--- app/controllers/snippets/notes_controller.rb | 1 + .../dm-notes-actions-noteable-for-update.yml | 5 +++++ spec/controllers/projects/notes_controller_spec.rb | 23 ++++++++++++++++++++++ 4 files changed, 36 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/dm-notes-actions-noteable-for-update.yml diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb index 3c64fd964ff..be2e1b47feb 100644 --- a/app/controllers/concerns/notes_actions.rb +++ b/app/controllers/concerns/notes_actions.rb @@ -4,7 +4,7 @@ module NotesActions included do before_action :set_polling_interval_header, only: [:index] - before_action :noteable, only: :index + before_action :require_noteable!, only: [:index, :create] before_action :authorize_admin_note!, only: [:update, :destroy] before_action :note_project, only: [:create] end @@ -90,7 +90,7 @@ module NotesActions if note.persisted? attrs[:valid] = true - if noteable.nil? || noteable.discussions_rendered_on_frontend? + if noteable.discussions_rendered_on_frontend? attrs.merge!(note_serializer.represent(note)) else attrs.merge!( @@ -191,7 +191,11 @@ module NotesActions end def noteable - @noteable ||= notes_finder.target || render_404 + @noteable ||= notes_finder.target || @note&.noteable + end + + def require_noteable! + render_404 unless noteable end def last_fetched_at diff --git a/app/controllers/snippets/notes_controller.rb b/app/controllers/snippets/notes_controller.rb index f9496787b15..c8b4682e6dc 100644 --- a/app/controllers/snippets/notes_controller.rb +++ b/app/controllers/snippets/notes_controller.rb @@ -20,6 +20,7 @@ class Snippets::NotesController < ApplicationController def snippet PersonalSnippet.find_by(id: params[:snippet_id]) end + alias_method :noteable, :snippet def note_params super.merge(noteable_id: params[:snippet_id]) diff --git a/changelogs/unreleased/dm-notes-actions-noteable-for-update.yml b/changelogs/unreleased/dm-notes-actions-noteable-for-update.yml new file mode 100644 index 00000000000..1d2f58bc765 --- /dev/null +++ b/changelogs/unreleased/dm-notes-actions-noteable-for-update.yml @@ -0,0 +1,5 @@ +--- +title: Make sure NotesActions#noteable returns a Noteable in the update action +merge_request: +author: +type: fixed diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index 5f5a789d5cc..37e9f863fc4 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -336,6 +336,29 @@ describe Projects::NotesController do end end + describe 'PUT update' do + let(:request_params) do + { + namespace_id: project.namespace, + project_id: project, + id: note, + format: :json, + note: { + note: "New comment" + } + } + end + + before do + sign_in(note.author) + project.team << [note.author, :developer] + end + + it "updates the note" do + expect { put :update, request_params }.to change { note.reload.note } + end + end + describe 'DELETE destroy' do let(:request_params) do { -- cgit v1.2.1 From b256c5e2a47d5a10d7d2ce682442560ff412b10a Mon Sep 17 00:00:00 2001 From: Winnie Hellmann Date: Thu, 16 Nov 2017 14:14:29 +0000 Subject: Update license_finder to 3.1.1 --- Gemfile | 2 +- Gemfile.lock | 12 ++++++++++-- config/dependency_decisions.yml | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index c495e4eceba..e357d76328a 100644 --- a/Gemfile +++ b/Gemfile @@ -343,7 +343,7 @@ group :development, :test do gem 'benchmark-ips', '~> 2.3.0', require: false - gem 'license_finder', '~> 2.1.0', require: false + gem 'license_finder', '~> 3.1', require: false gem 'knapsack', '~> 1.11.0' gem 'activerecord_sane_schema_dumper', '0.2' diff --git a/Gemfile.lock b/Gemfile.lock index 43555a01037..dc56e6e8f82 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -83,6 +83,7 @@ GEM bindata (2.4.1) binding_of_caller (0.7.2) debug_inspector (>= 0.0.1) + blankslate (2.1.2.4) bootstrap-sass (3.3.6) autoprefixer-rails (>= 5.2.1) sass (>= 3.3.4) @@ -451,11 +452,13 @@ GEM actionmailer (>= 3.2) letter_opener (~> 1.0) railties (>= 3.2) - license_finder (2.1.0) + license_finder (3.1.1) bundler httparty rubyzip thor + toml (= 0.1.2) + with_env (> 1.0) xml-simple licensee (8.7.0) rugged (~> 0.24) @@ -571,6 +574,8 @@ GEM activerecord (>= 4.0, < 5.2) parser (2.4.0.0) ast (~> 2.2) + parslet (1.5.0) + blankslate (~> 2.0) path_expander (1.0.1) peek (1.0.1) concurrent-ruby (>= 0.9.0) @@ -898,6 +903,8 @@ GEM tilt (2.0.6) timecop (0.8.1) timfel-krb5-auth (0.8.3) + toml (0.1.2) + parslet (~> 1.5.0) toml-rb (0.3.15) citrus (~> 3.0, > 3.0) truncato (0.7.10) @@ -952,6 +959,7 @@ GEM builder expression_parser rinku + with_env (1.1.0) xml-simple (1.1.5) xpath (2.1.0) nokogiri (~> 1.3) @@ -1058,7 +1066,7 @@ DEPENDENCIES knapsack (~> 1.11.0) kubeclient (~> 2.2.0) letter_opener_web (~> 1.3.0) - license_finder (~> 2.1.0) + license_finder (~> 3.1) licensee (~> 8.7.0) lograge (~> 0.5) loofah (~> 2.0.3) diff --git a/config/dependency_decisions.yml b/config/dependency_decisions.yml index 4004e4d767d..60df92a44fc 100644 --- a/config/dependency_decisions.yml +++ b/config/dependency_decisions.yml @@ -471,3 +471,35 @@ :why: :versions: [] :when: 2017-10-17 17:46:12.367554000 Z +- - :license + - component-emitter + - MIT + - :who: Winnie Hellmann + :why: package.json does not specify the license (README.md does) + :versions: + - 1.1.2 + :when: 2017-11-13 12:23:10.502463000 Z +- - :license + - json-schema + - BSD + - :who: Winnie Hellmann + :why: https://github.com/kriszyp/json-schema/blob/v0.2.3/package.json#L18-L19 + :versions: + - 0.2.3 + :when: 2017-11-16 12:52:18.286091000 Z +- - :license + - node-forge + - New BSD + - :who: Winnie Hellmann + :why: https://github.com/digitalbazaar/forge/blob/0.6.33/LICENSE + :versions: + - 0.6.33 + :when: 2017-11-16 12:56:17.974767000 Z +- - :license + - sntp + - BSD + - :who: Winnie Hellmann + :why: https://github.com/hueniverse/sntp/blob/v1.0.9/package.json#L28-L29 + :versions: + - 1.0.9 + :when: 2017-11-16 13:02:06.765282000 Z -- cgit v1.2.1 From 71b2cc1dd8497959306601eece8ebbf008562d07 Mon Sep 17 00:00:00 2001 From: "micael.bergeron" Date: Thu, 16 Nov 2017 09:23:32 -0500 Subject: reverting to the simpler approach --- app/models/commit.rb | 2 +- app/models/note.rb | 14 +++++++++++++- app/models/system_note_metadata.rb | 10 ++++++++++ app/services/system_note_service.rb | 4 ++++ spec/models/commit_spec.rb | 23 ----------------------- 5 files changed, 28 insertions(+), 25 deletions(-) diff --git a/app/models/commit.rb b/app/models/commit.rb index b5411b2bf75..6dba154a6ea 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -26,7 +26,7 @@ class Commit DIFF_HARD_LIMIT_LINES = 50000 MIN_SHA_LENGTH = 7 - COMMIT_SHA_PATTERN = /(? Date: Fri, 10 Nov 2017 20:57:11 +0100 Subject: Optimise getting the pipeline status of commits This adds an optimised way of getting the latest pipeline status for a list of Commit objects (or just a single one). --- app/controllers/projects/commits_controller.rb | 1 + .../projects/merge_requests_controller.rb | 3 +- app/models/ci/pipeline.rb | 66 ++++++++--- app/models/commit.rb | 9 +- app/models/commit_collection.rb | 44 ++++++++ app/models/merge_request_diff.rb | 4 +- app/models/repository.rb | 16 ++- changelogs/unreleased/ci-pipeline-status-query.yml | 5 + spec/models/ci/pipeline_spec.rb | 122 ++++++++++++++------- spec/models/commit_collection_spec.rb | 59 ++++++++++ spec/models/commit_spec.rb | 11 +- 11 files changed, 274 insertions(+), 66 deletions(-) create mode 100644 app/models/commit_collection.rb create mode 100644 changelogs/unreleased/ci-pipeline-status-query.yml create mode 100644 spec/models/commit_collection_spec.rb diff --git a/app/controllers/projects/commits_controller.rb b/app/controllers/projects/commits_controller.rb index 28920877635..5f4afd2cdee 100644 --- a/app/controllers/projects/commits_controller.rb +++ b/app/controllers/projects/commits_controller.rb @@ -57,6 +57,7 @@ class Projects::CommitsController < Projects::ApplicationController @repository.commits(@ref, path: @path, limit: @limit, offset: @offset) end + @commits = @commits.with_pipeline_status @commits = prepare_commits_for_rendering(@commits) end end diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 22de6680511..abe4e5245b1 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -80,7 +80,8 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo def commits # Get commits from repository # or from cache if already merged - @commits = prepare_commits_for_rendering(@merge_request.commits) + @commits = + prepare_commits_for_rendering(@merge_request.commits.with_pipeline_status) render json: { html: view_to_html_string('projects/merge_requests/_commits') } end diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 19814864e50..c77c837ff3f 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -149,34 +149,70 @@ module Ci end end - # ref can't be HEAD or SHA, can only be branch/tag name - scope :latest, ->(ref = nil) do - max_id = unscope(:select) - .select("max(#{quoted_table_name}.id)") - .group(:ref, :sha) - - if ref - where(ref: ref, id: max_id.where(ref: ref)) - else - where(id: max_id) - end - end scope :internal, -> { where(source: internal_sources) } + # Returns the pipelines in descending order (= newest first), optionally + # limited to a number of references. + # + # ref - The name (or names) of the branch(es)/tag(s) to limit the list of + # pipelines to. + def self.newest_first(ref = nil) + relation = order(id: :desc) + + ref ? relation.where(ref: ref) : relation + end + def self.latest_status(ref = nil) - latest(ref).status + newest_first(ref).pluck(:status).first end def self.latest_successful_for(ref) - success.latest(ref).order(id: :desc).first + newest_first(ref).success.take end def self.latest_successful_for_refs(refs) - success.latest(refs).order(id: :desc).each_with_object({}) do |pipeline, hash| + relation = newest_first(refs).success + + relation.each_with_object({}) do |pipeline, hash| hash[pipeline.ref] ||= pipeline end end + # Returns a Hash containing the latest pipeline status for every given + # commit. + # + # The keys of this Hash are the commit SHAs, the values the statuses. + # + # commits - The list of commit SHAs to get the status for. + # ref - The ref to scope the data to (e.g. "master"). If the ref is not + # given we simply get the latest status for the commits, regardless + # of what refs their pipelines belong to. + def self.latest_status_per_commit(commits, ref = nil) + p1 = arel_table + p2 = arel_table.alias + + # This LEFT JOIN will filter out all but the newest row for every + # combination of (project_id, sha) or (project_id, sha, ref) if a ref is + # given. + cond = p1[:sha].eq(p2[:sha]) + .and(p1[:project_id].eq(p2[:project_id])) + .and(p1[:id].lt(p2[:id])) + + cond = cond.and(p1[:ref].eq(p2[:ref])) if ref + join = p1.join(p2, Arel::Nodes::OuterJoin).on(cond) + + relation = select(:sha, :status) + .where(sha: commits) + .where(p2[:id].eq(nil)) + .joins(join.join_sources) + + relation = relation.where(ref: ref) if ref + + relation.each_with_object({}) do |row, hash| + hash[row[:sha]] = row[:status] + end + end + def self.truncate_sha(sha) sha[0...8] end diff --git a/app/models/commit.rb b/app/models/commit.rb index 6dba154a6ea..a31ebe9cc87 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -80,6 +80,7 @@ class Commit @raw = raw_commit @project = project + @statuses = {} end def id @@ -236,11 +237,13 @@ class Commit end def status(ref = nil) - @statuses ||= {} - return @statuses[ref] if @statuses.key?(ref) - @statuses[ref] = pipelines.latest_status(ref) + @statuses[ref] = project.pipelines.latest_status_per_commit(id, ref)[id] + end + + def set_status_for_ref(ref, status) + @statuses[ref] = status end def signature diff --git a/app/models/commit_collection.rb b/app/models/commit_collection.rb new file mode 100644 index 00000000000..dd93af9df64 --- /dev/null +++ b/app/models/commit_collection.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +# A collection of Commit instances for a specific project and Git reference. +class CommitCollection + include Enumerable + + attr_reader :project, :ref, :commits + + # project - The project the commits belong to. + # commits - The Commit instances to store. + # ref - The name of the ref (e.g. "master"). + def initialize(project, commits, ref = nil) + @project = project + @commits = commits + @ref = ref + end + + def each(&block) + commits.each(&block) + end + + # Sets the pipeline status for every commit. + # + # Setting this status ahead of time removes the need for running a query for + # every commit we're displaying. + def with_pipeline_status + statuses = project.pipelines.latest_status_per_commit(map(&:id), ref) + + each do |commit| + commit.set_status_for_ref(ref, statuses[commit.id]) + end + + self + end + + def respond_to_missing?(message, inc_private = false) + commits.respond_to?(message, inc_private) + end + + # rubocop:disable GitlabSecurity/PublicSend + def method_missing(message, *args, &block) + commits.public_send(message, *args, &block) + end +end diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 1eda0f9cbbd..5382f5cc627 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -284,8 +284,10 @@ class MergeRequestDiff < ActiveRecord::Base def load_commits commits = st_commits.presence || merge_request_diff_commits + commits = commits.map { |commit| Commit.from_hash(commit.to_hash, project) } - commits.map { |commit| Commit.from_hash(commit.to_hash, project) } + CommitCollection + .new(merge_request.source_project, commits, merge_request.source_branch) end def save_diffs diff --git a/app/models/repository.rb b/app/models/repository.rb index 3a89fa9264b..35ee12bdfa1 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -132,7 +132,8 @@ class Repository commits = Gitlab::Git::Commit.where(options) commits = Commit.decorate(commits, @project) if commits.present? - commits + + CommitCollection.new(project, commits, ref) end def commits_between(from, to) @@ -148,11 +149,14 @@ class Repository end raw_repository.gitaly_migrate(:commits_by_message) do |is_enabled| - if is_enabled - find_commits_by_message_by_gitaly(query, ref, path, limit, offset) - else - find_commits_by_message_by_shelling_out(query, ref, path, limit, offset) - end + commits = + if is_enabled + find_commits_by_message_by_gitaly(query, ref, path, limit, offset) + else + find_commits_by_message_by_shelling_out(query, ref, path, limit, offset) + end + + CommitCollection.new(project, commits, ref) end end diff --git a/changelogs/unreleased/ci-pipeline-status-query.yml b/changelogs/unreleased/ci-pipeline-status-query.yml new file mode 100644 index 00000000000..a464e501418 --- /dev/null +++ b/changelogs/unreleased/ci-pipeline-status-query.yml @@ -0,0 +1,5 @@ +--- +title: Optimise getting the pipeline status of commits +merge_request: +author: +type: performance diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 2c9e7013b77..b89b0e555d9 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -625,38 +625,29 @@ describe Ci::Pipeline, :mailer do shared_context 'with some outdated pipelines' do before do - create_pipeline(:canceled, 'ref', 'A') - create_pipeline(:success, 'ref', 'A') - create_pipeline(:failed, 'ref', 'B') - create_pipeline(:skipped, 'feature', 'C') + create_pipeline(:canceled, 'ref', 'A', project) + create_pipeline(:success, 'ref', 'A', project) + create_pipeline(:failed, 'ref', 'B', project) + create_pipeline(:skipped, 'feature', 'C', project) end - def create_pipeline(status, ref, sha) - create(:ci_empty_pipeline, status: status, ref: ref, sha: sha) + def create_pipeline(status, ref, sha, project) + create( + :ci_empty_pipeline, + status: status, + ref: ref, + sha: sha, + project: project + ) end end - describe '.latest' do + describe '.newest_first' do include_context 'with some outdated pipelines' - context 'when no ref is specified' do - let(:pipelines) { described_class.latest.all } - - it 'returns the latest pipeline for the same ref and different sha' do - expect(pipelines.map(&:sha)).to contain_exactly('A', 'B', 'C') - expect(pipelines.map(&:status)) - .to contain_exactly('success', 'failed', 'skipped') - end - end - - context 'when ref is specified' do - let(:pipelines) { described_class.latest('ref').all } - - it 'returns the latest pipeline for ref and different sha' do - expect(pipelines.map(&:sha)).to contain_exactly('A', 'B') - expect(pipelines.map(&:status)) - .to contain_exactly('success', 'failed') - end + it 'returns the pipelines from new to old' do + expect(described_class.newest_first.pluck(:status)) + .to eq(%w[skipped failed success canceled]) end end @@ -664,20 +655,14 @@ describe Ci::Pipeline, :mailer do include_context 'with some outdated pipelines' context 'when no ref is specified' do - let(:latest_status) { described_class.latest_status } - - it 'returns the latest status for the same ref and different sha' do - expect(latest_status).to eq(described_class.latest.status) - expect(latest_status).to eq('failed') + it 'returns the status of the latest pipeline' do + expect(described_class.latest_status).to eq('skipped') end end context 'when ref is specified' do - let(:latest_status) { described_class.latest_status('ref') } - - it 'returns the latest status for ref and different sha' do - expect(latest_status).to eq(described_class.latest_status('ref')) - expect(latest_status).to eq('failed') + it 'returns the status of the latest pipeline for the given ref' do + expect(described_class.latest_status('ref')).to eq('failed') end end end @@ -686,7 +671,7 @@ describe Ci::Pipeline, :mailer do include_context 'with some outdated pipelines' let!(:latest_successful_pipeline) do - create_pipeline(:success, 'ref', 'D') + create_pipeline(:success, 'ref', 'D', project) end it 'returns the latest successful pipeline' do @@ -698,8 +683,13 @@ describe Ci::Pipeline, :mailer do describe '.latest_successful_for_refs' do include_context 'with some outdated pipelines' - let!(:latest_successful_pipeline1) { create_pipeline(:success, 'ref1', 'D') } - let!(:latest_successful_pipeline2) { create_pipeline(:success, 'ref2', 'D') } + let!(:latest_successful_pipeline1) do + create_pipeline(:success, 'ref1', 'D', project) + end + + let!(:latest_successful_pipeline2) do + create_pipeline(:success, 'ref2', 'D', project) + end it 'returns the latest successful pipeline for both refs' do refs = %w(ref1 ref2 ref3) @@ -708,6 +698,62 @@ describe Ci::Pipeline, :mailer do end end + describe '.latest_status_per_commit' do + let(:project) { create(:project) } + + before do + pairs = [ + %w[success ref1 123], + %w[manual master 123], + %w[failed ref 456] + ] + + pairs.each do |(status, ref, sha)| + create( + :ci_empty_pipeline, + status: status, + ref: ref, + sha: sha, + project: project + ) + end + end + + context 'without a ref' do + it 'returns a Hash containing the latest status per commit for all refs' do + expect(described_class.latest_status_per_commit(%w[123 456])) + .to eq({ '123' => 'manual', '456' => 'failed' }) + end + + it 'only includes the status of the given commit SHAs' do + expect(described_class.latest_status_per_commit(%w[123])) + .to eq({ '123' => 'manual' }) + end + + context 'when there are two pipelines for a ref and SHA' do + it 'returns the status of the latest pipeline' do + create( + :ci_empty_pipeline, + status: 'failed', + ref: 'master', + sha: '123', + project: project + ) + + expect(described_class.latest_status_per_commit(%w[123])) + .to eq({ '123' => 'failed' }) + end + end + end + + context 'with a ref' do + it 'only includes the pipelines for the given ref' do + expect(described_class.latest_status_per_commit(%w[123 456], 'master')) + .to eq({ '123' => 'manual' }) + end + end + end + describe '.internal_sources' do subject { described_class.internal_sources } diff --git a/spec/models/commit_collection_spec.rb b/spec/models/commit_collection_spec.rb new file mode 100644 index 00000000000..066fe7d154e --- /dev/null +++ b/spec/models/commit_collection_spec.rb @@ -0,0 +1,59 @@ +require 'spec_helper' + +describe CommitCollection do + let(:project) { create(:project, :repository) } + let(:commit) { project.commit } + + describe '#each' do + it 'yields every commit' do + collection = described_class.new(project, [commit]) + + expect { |b| collection.each(&b) }.to yield_with_args(commit) + end + end + + describe '#with_pipeline_status' do + it 'sets the pipeline status for every commit so no additional queries are necessary' do + create( + :ci_empty_pipeline, + ref: 'master', + sha: commit.id, + status: 'success', + project: project + ) + + collection = described_class.new(project, [commit]) + collection.with_pipeline_status + + recorder = ActiveRecord::QueryRecorder.new do + expect(commit.status).to eq('success') + end + + expect(recorder.count).to be_zero + end + end + + describe '#respond_to_missing?' do + it 'returns true when the underlying Array responds to the message' do + collection = described_class.new(project, []) + + expect(collection.respond_to?(:last)).to eq(true) + end + + it 'returns false when the underlying Array does not respond to the message' do + collection = described_class.new(project, []) + + expect(collection.respond_to?(:foo)).to eq(false) + end + end + + describe '#method_missing' do + it 'delegates undefined methods to the underlying Array' do + collection = described_class.new(project, [commit]) + + expect(collection.length).to eq(1) + expect(collection.last).to eq(commit) + expect(collection).not_to be_empty + end + end +end diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index e3cfa149e3a..d18a5c9dfa6 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -351,12 +351,19 @@ eos end it 'gives compound status from latest pipelines if ref is nil' do - expect(commit.status(nil)).to eq(Ci::Pipeline.latest_status) - expect(commit.status(nil)).to eq('failed') + expect(commit.status(nil)).to eq(pipeline_from_fix.status) end end end + describe '#set_status_for_ref' do + it 'sets the status for a given reference' do + commit.set_status_for_ref('master', 'failed') + + expect(commit.status('master')).to eq('failed') + end + end + describe '#participants' do let(:user1) { build(:user) } let(:user2) { build(:user) } -- cgit v1.2.1 From 011558ea4ac59bce74c18d2f7c55ac257de111c6 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Thu, 16 Nov 2017 12:49:01 -0200 Subject: Add logs for monitoring the merge process --- app/services/merge_requests/merge_service.rb | 12 ++++++++++++ changelogs/unreleased/osw-merge-process-logs.yml | 5 +++++ 2 files changed, 17 insertions(+) create mode 100644 changelogs/unreleased/osw-merge-process-logs.yml diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 156e7b2f078..ac96182e473 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -10,6 +10,8 @@ module MergeRequests attr_reader :merge_request, :source + delegate :merge_jid, :state, to: :@merge_request + def execute(merge_request) if project.merge_requests_ff_only_enabled && !self.is_a?(FfMergeService) FfMergeService.new(project, current_user, params).execute(merge_request) @@ -35,6 +37,7 @@ module MergeRequests success end end + log_info("Merge process finished on JID #{merge_jid} with state #{state}") rescue MergeError => e handle_merge_error(log_message: e.message, save_message_on_model: true) end @@ -44,7 +47,9 @@ module MergeRequests def commit message = params[:commit_message] || merge_request.merge_commit_message + log_info("Git merge started on JID #{merge_jid}") commit_id = repository.merge(current_user, source, merge_request, message) + log_info("Git merge finished on JID #{merge_jid} commit #{commit_id}") raise MergeError, 'Conflicts detected during merge' unless commit_id @@ -58,7 +63,9 @@ module MergeRequests end def after_merge + log_info("Post merge started on JID #{merge_jid} with state #{state}") MergeRequests::PostMergeService.new(project, current_user).execute(merge_request) + log_info("Post merge finished on JID #{merge_jid} with state #{state}") if delete_source_branch? DeleteBranchService.new(@merge_request.source_project, branch_deletion_user) @@ -94,5 +101,10 @@ module MergeRequests def find_merge_source merge_request.diff_head_sha end + + def log_info(message) + @logger ||= Rails.logger + @logger.info(message) + end end end diff --git a/changelogs/unreleased/osw-merge-process-logs.yml b/changelogs/unreleased/osw-merge-process-logs.yml new file mode 100644 index 00000000000..d2bb0e09834 --- /dev/null +++ b/changelogs/unreleased/osw-merge-process-logs.yml @@ -0,0 +1,5 @@ +--- +title: Add logs for monitoring the merge process +merge_request: +author: +type: other -- cgit v1.2.1 From 08eaa59359ff1afc8ecac5b942d0f5b355cb8df4 Mon Sep 17 00:00:00 2001 From: Tim Zallmann Date: Fri, 3 Nov 2017 09:44:00 +0100 Subject: Added + Updated Document for Vue Component --- .../javascripts/vue_shared/components/icon.vue | 7 +++--- doc/development/fe_guide/icons.md | 28 ++++++++++++++++++---- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/vue_shared/components/icon.vue b/app/assets/javascripts/vue_shared/components/icon.vue index 2e5f9f1088f..8f116233e72 100644 --- a/app/assets/javascripts/vue_shared/components/icon.vue +++ b/app/assets/javascripts/vue_shared/components/icon.vue @@ -6,10 +6,9 @@ Sample configuration: */ diff --git a/doc/development/fe_guide/icons.md b/doc/development/fe_guide/icons.md index cef62618a3c..382c7311905 100644 --- a/doc/development/fe_guide/icons.md +++ b/doc/development/fe_guide/icons.md @@ -4,15 +4,17 @@ We are using SVG Icons in GitLab with a SVG Sprite, due to this the icons are on ### Usage in HAML/Rails -To use a sprite Icon in HAML or Rails we use a specific helper function : +To use a sprite Icon in HAML or Rails we use a specific helper function : `sprite_icon(icon_name, size: nil, css_class: '')` -**icon_name** Use the icon_name that you can find in the SVG Sprite (Overview is available under `/assets/sprite.symbol.html`). +**icon_name** Use the icon_name that you can find in the SVG Sprite ([Overview is available here](http://gitlab-org.gitlab.io/gitlab-svgs/)`). + **size (optional)** Use one of the following sizes : 16,24,32,48,72 (this will be translated into a `s16` class) + **css_class (optional)** If you want to add additional css classes -**Example** +**Example** `= sprite_icon('issues', size: 72, css_class: 'icon-danger')` @@ -20,9 +22,27 @@ To use a sprite Icon in HAML or Rails we use a specific helper function : `` +### Usage in Vue + +We have a special Vue component for our sprite icons in `\vue_shared\components\icon.vue`. + +Sample usage : + +`` + +**name** Name of the Icon in the SVG Sprite ([Overview is available here](http://gitlab-org.gitlab.io/gitlab-svgs/)`). + +**size (optional)** Number value for the size which is then mapped to a specific CSS class (Available Sizes: 8,12,16,18,24,32,48,72 are mapped to `sXX` css classes) + +**css-classes (optional)** Additional CSS Classes to add to the svg tag. + ### Usage in HTML/JS -Please use the following function inside JS to render an icon : +Please use the following function inside JS to render an icon : `gl.utils.spriteIcon(iconName)` ## Adding a new icon to the sprite -- cgit v1.2.1 From 245d0daddef23427d9b1aa206dbedf5b9cc89b34 Mon Sep 17 00:00:00 2001 From: Tim Zallmann Date: Tue, 14 Nov 2017 20:08:51 +0100 Subject: Added info about tracking --- doc/development/fe_guide/icons.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/development/fe_guide/icons.md b/doc/development/fe_guide/icons.md index 382c7311905..b288ee95722 100644 --- a/doc/development/fe_guide/icons.md +++ b/doc/development/fe_guide/icons.md @@ -49,7 +49,7 @@ Please use the following function inside JS to render an icon : All Icons and Illustrations are managed in the [gitlab-svgs](https://gitlab.com/gitlab-org/gitlab-svgs) repository which is added as a dev-dependency. -To upgrade to a new SVG Sprite version run `yarn upgrade @gitlab-org/gitlab-svgs` and then run `yarn run svg`. This task will copy the svg sprite and all illustrations in the correct folders. +To upgrade to a new SVG Sprite version run `yarn upgrade @gitlab-org/gitlab-svgs` and then run `yarn run svg`. This task will copy the svg sprite and all illustrations in the correct folders. The updated files should be tracked in Git as those are referenced. # SVG Illustrations -- cgit v1.2.1 From 555f261d73aa7d5765f0d57fc9773898d3212ee9 Mon Sep 17 00:00:00 2001 From: Lee Matos Date: Thu, 16 Nov 2017 15:49:41 +0000 Subject: Update HA README.md to clarify GitLab support does not troubleshoot DRBD. --- doc/administration/high_availability/README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/administration/high_availability/README.md b/doc/administration/high_availability/README.md index 4d3be0ab8f6..a88e67bfeb5 100644 --- a/doc/administration/high_availability/README.md +++ b/doc/administration/high_availability/README.md @@ -53,7 +53,9 @@ or in different cloud availability zones. > **Note:** GitLab recommends against choosing this HA method because of the complexity of managing DRBD and crafting automatic failover. This is - *compatible* with GitLab, but not officially *supported*. + *compatible* with GitLab, but not officially *supported*. If you are + an EE customer, support will help you with GitLab related problems, but if the + root cause is identified as DRBD, we will not troubleshoot further. Components/Servers Required: 2 servers/virtual machines (one active/one passive) -- cgit v1.2.1 From 869fc05d81eaced5d68ef28e334ae9ece80f74f7 Mon Sep 17 00:00:00 2001 From: "micael.bergeron" Date: Thu, 16 Nov 2017 11:03:15 -0500 Subject: fix the linting error --- app/models/system_note_metadata.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb index 4065c1594c3..ca0673080af 100644 --- a/app/models/system_note_metadata.rb +++ b/app/models/system_note_metadata.rb @@ -7,7 +7,7 @@ class SystemNoteMetadata < ActiveRecord::Base TYPES_WITH_CROSS_REFERENCES = %w[ cross_reference milestone - ] + ].freeze ICON_TYPES = %w[ commit description merge confidential visible label assignee cross_reference -- cgit v1.2.1 From d0f7e4df349b8095eb6ea9d7b6b9b234e0c5dd58 Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Thu, 16 Nov 2017 16:44:16 +0000 Subject: Resolve "lock/confidential issuable sidebar custom svg icons iteration" --- .../components/issue_discussion_locked_widget.vue | 17 +++++++----- .../confidential/confidential_issue_sidebar.vue | 29 ++++++++++++++------ .../sidebar/components/lock/lock_issue_sidebar.vue | 31 +++++++++++++--------- .../vue_shared/components/issue/issue_warning.vue | 29 ++++++++++++-------- app/assets/stylesheets/pages/issuable.scss | 18 ++++--------- app/assets/stylesheets/pages/note_form.scss | 20 +++++++++++--- app/views/projects/issues/show.html.haml | 4 +-- .../projects/merge_requests/_mr_title.html.haml | 2 +- .../components/issue/issue_warning_spec.js | 6 ++--- 9 files changed, 96 insertions(+), 60 deletions(-) diff --git a/app/assets/javascripts/notes/components/issue_discussion_locked_widget.vue b/app/assets/javascripts/notes/components/issue_discussion_locked_widget.vue index e73ec2aaf71..64466b04b40 100644 --- a/app/assets/javascripts/notes/components/issue_discussion_locked_widget.vue +++ b/app/assets/javascripts/notes/components/issue_discussion_locked_widget.vue @@ -1,18 +1,21 @@