From fc2ec3b5b6b5bf07ff911f62641144fdb9bf334a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 11 Dec 2018 12:43:54 +0000 Subject: [PATCH 01/47] Merge branch 'fix-mr-pipelines-run-on-regex' into 'master' Fix MR pipelines run on only: regexp Closes #55026 See merge request gitlab-org/gitlab-ce!23657 (cherry picked from commit 7e4fcfb04c70604a8d9c2fade167bb04fc8f1d28) eb1956c5 Fix MR pipelines run on only: refex --- lib/gitlab/ci/build/policy/refs.rb | 12 ++-- .../ci/create_pipeline_service_spec.rb | 58 +++++++++++++++++++ 2 files changed, 66 insertions(+), 4 deletions(-) diff --git a/lib/gitlab/ci/build/policy/refs.rb b/lib/gitlab/ci/build/policy/refs.rb index 10934536536..0e9bb5c94bb 100644 --- a/lib/gitlab/ci/build/policy/refs.rb +++ b/lib/gitlab/ci/build/policy/refs.rb @@ -32,10 +32,14 @@ module Gitlab return true if pipeline.source == pattern return true if pipeline.source&.pluralize == pattern - if pattern.first == "/" && pattern.last == "/" - Regexp.new(pattern[1...-1]) =~ pipeline.ref - else - pattern == pipeline.ref + # patterns can be matched only when branch or tag is used + # the pattern matching does not work for merge requests pipelines + if pipeline.branch? || pipeline.tag? + if pattern.first == "/" && pattern.last == "/" + Regexp.new(pattern[1...-1]) =~ pipeline.ref + else + pattern == pipeline.ref + end end end end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index ccc6b0ef1c7..8b8021ecbc8 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -810,6 +810,64 @@ describe Ci::CreatePipelineService do end end end + + context "when config uses regular expression for only keyword" do + let(:config) do + { + build: { + stage: 'build', + script: 'echo', + only: ["/^#{ref_name}$/"] + } + } + end + + context 'when merge request is specified' do + let(:merge_request) do + create(:merge_request, + source_project: project, + source_branch: ref_name, + target_project: project, + target_branch: 'master') + end + + it 'does not create a merge request pipeline' do + expect(pipeline).not_to be_persisted + + expect(pipeline.errors[:base]) + .to eq(['No stages / jobs for this pipeline.']) + end + end + end + + context "when config has 'except: [tags]'" do + let(:config) do + { + build: { + stage: 'build', + script: 'echo', + except: ['tags'] + } + } + end + + context 'when merge request is specified' do + let(:merge_request) do + create(:merge_request, + source_project: project, + source_branch: ref_name, + target_project: project, + target_branch: 'master') + end + + it 'does not create a merge request pipeline' do + expect(pipeline).not_to be_persisted + + expect(pipeline.errors[:base]) + .to eq(['No stages / jobs for this pipeline.']) + end + end + end end context 'when source is web' do -- GitLab From e215f9dd8b5604836410653147ebb7afaa15a87b Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Wed, 12 Dec 2018 16:15:58 +0000 Subject: [PATCH 02/47] Merge branch 'issue_55024' into 'master' Do not save user preferences on read-only mode Closes #55024 See merge request gitlab-org/gitlab-ce!23698 (cherry picked from commit c7ba5c0aeb808f4dc057428a2a98fec7fa1939fb) 4528c625 Do not save user preferences on read-only mode bf1bf14d Address review comment 30642732 Improve specs --- .../concerns/issuable_collections.rb | 2 ++ .../projects/issues_controller_spec.rb | 2 ++ .../merge_requests_controller_spec.rb | 2 ++ ...er_from_user_preference_shared_examples.rb | 32 +++++++++++++++++++ 4 files changed, 38 insertions(+) create mode 100644 spec/support/shared_examples/controllers/set_sort_order_from_user_preference_shared_examples.rb diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index a597996a362..789e0dc736e 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -126,6 +126,8 @@ module IssuableCollections sort_param = params[:sort] sort_param ||= user_preference[issuable_sorting_field] + return sort_param if Gitlab::Database.read_only? + if user_preference[issuable_sorting_field] != sort_param user_preference.update_attribute(issuable_sorting_field, sort_param) end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 02930edbf72..6240ab6d867 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -42,6 +42,8 @@ describe Projects::IssuesController do it_behaves_like "issuables list meta-data", :issue + it_behaves_like 'set sort order from user preference' + it "returns index" do get :index, namespace_id: project.namespace, project_id: project diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 7f15da859e5..a37a831ddbb 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -160,6 +160,8 @@ describe Projects::MergeRequestsController do it_behaves_like "issuables list meta-data", :merge_request + it_behaves_like 'set sort order from user preference' + context 'when page param' do let(:last_page) { project.merge_requests.page().total_pages } let!(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) } diff --git a/spec/support/shared_examples/controllers/set_sort_order_from_user_preference_shared_examples.rb b/spec/support/shared_examples/controllers/set_sort_order_from_user_preference_shared_examples.rb new file mode 100644 index 00000000000..b34948be670 --- /dev/null +++ b/spec/support/shared_examples/controllers/set_sort_order_from_user_preference_shared_examples.rb @@ -0,0 +1,32 @@ +shared_examples 'set sort order from user preference' do + describe '#set_sort_order_from_user_preference' do + # There is no issuable_sorting_field defined in any CE controllers yet, + # however any other field present in user_preferences table can be used for testing. + let(:sorting_field) { :issue_notes_filter } + let(:sorting_param) { 'any' } + + before do + allow(controller).to receive(:issuable_sorting_field).and_return(sorting_field) + end + + context 'when database is in read-only mode' do + it 'it does not update user preference' do + allow(Gitlab::Database).to receive(:read_only?).and_return(true) + + expect_any_instance_of(UserPreference).not_to receive(:update_attribute).with(sorting_field, sorting_param) + + get :index, namespace_id: project.namespace, project_id: project, sort: sorting_param + end + end + + context 'when database is not in read-only mode' do + it 'updates user preference' do + allow(Gitlab::Database).to receive(:read_only?).and_return(false) + + expect_any_instance_of(UserPreference).to receive(:update_attribute).with(sorting_field, sorting_param) + + get :index, namespace_id: project.namespace, project_id: project, sort: sorting_param + end + end + end +end -- GitLab From 26cf65c06fe67c708738e6e74dd7a66b79783016 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Wed, 12 Dec 2018 13:44:40 +0000 Subject: [PATCH 03/47] Merge branch 'zj-fix-pool-creation' into 'master' Fix creation query for pools repository See merge request gitlab-org/gitlab-ce!23729 (cherry picked from commit 460eb38ce53acff9e4db003eaffa889e6ff40987) 2fa9f305 Fix creation query for pools repository 5db7a7df Update project_spec.rb for rubocop --- app/models/project.rb | 6 +++--- spec/models/project_spec.rb | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index f5dc58cd67f..59965cd507e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -2009,12 +2009,12 @@ class Project < ActiveRecord::Base def create_new_pool_repository pool = begin - create_or_find_pool_repository!(shard: Shard.by_name(repository_storage), source_project: self) + create_pool_repository!(shard: Shard.by_name(repository_storage), source_project: self) rescue ActiveRecord::RecordNotUnique - retry + pool_repository(true) end - pool.schedule + pool.schedule unless pool.scheduled? pool end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 1c85411dc3b..1a7e40dfebb 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -4092,6 +4092,29 @@ describe Project do end end + describe '#object_pool_params' do + let(:project) { create(:project, :repository, :public) } + + subject { project.object_pool_params } + + before do + stub_application_setting(hashed_storage_enabled: true) + end + + context 'when the objects cannot be pooled' do + let(:project) { create(:project, :repository, :private) } + + it { is_expected.to be_empty } + end + + context 'when a pool is created' do + it 'returns that pool repository' do + expect(subject).not_to be_empty + expect(subject[:pool_repository]).to be_persisted + end + end + end + describe '#git_objects_poolable?' do subject { project } -- GitLab From 5035156da2a392301641583d6a72d3b902e0463f Mon Sep 17 00:00:00 2001 From: Marcia Ramos Date: Tue, 11 Dec 2018 20:07:23 +0000 Subject: [PATCH 04/47] Merge branch 'ee-fj-web-ide-terminal-docs' into 'master' Docs: update Web IDE docs See merge request gitlab-org/gitlab-ce!23735 (cherry picked from commit 1ee3c06f072924fbad64b2d447f4eb835b8427fd) 30e3f02a Use feature name as h1, use inline links 7e94b208 Fixes syntax in "introduced in" notes --- doc/ci/interactive_web_terminal/index.md | 8 +++----- doc/user/project/web_ide/index.md | 12 ++++++------ 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/doc/ci/interactive_web_terminal/index.md b/doc/ci/interactive_web_terminal/index.md index 2c799e83a5f..d8136b80c11 100644 --- a/doc/ci/interactive_web_terminal/index.md +++ b/doc/ci/interactive_web_terminal/index.md @@ -1,4 +1,4 @@ -# Getting started with interactive web terminals **[CORE ONLY]** +# Interactive Web Terminals **[CORE ONLY]** > [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/50144) in GitLab 11.3. @@ -15,7 +15,7 @@ progress. Two things need to be configured for the interactive web terminal to work: - The Runner needs to have [`[session_server]` configured - properly][session-server] + properly](https://docs.gitlab.com/runner/configuration/advanced-configuration.html#the-session_server-section) - If you are using a reverse proxy with your GitLab instance, web terminals need to be [enabled](../../administration/integration/terminal.md#enabling-and-disabling-terminal-support) @@ -45,9 +45,7 @@ the terminal and type commands like a normal shell. If you have the terminal open and the job has finished with its tasks, the terminal will block the job from finishing for the duration configured in -[`[session_server].terminal_max_retention_time`][session-server] until you +[`[session_server].terminal_max_retention_time`](https://docs.gitlab.com/runner/configuration/advanced-configuration.html#the-session_server-section) until you close the terminal window. ![finished job with terminal open](img/finished_job_with_terminal_open.png) - -[session-server]: https://docs.gitlab.com/runner/configuration/advanced-configuration.html#the-session_server-section diff --git a/doc/user/project/web_ide/index.md b/doc/user/project/web_ide/index.md index e6b1f6b6aae..55e53b865af 100644 --- a/doc/user/project/web_ide/index.md +++ b/doc/user/project/web_ide/index.md @@ -1,6 +1,6 @@ # Web IDE -> [Introduced in](https://gitlab.com/gitlab-org/gitlab-ee/issues/4539) [GitLab Ultimate][ee] 10.4. +> [Introduced](https://gitlab.com/gitlab-org/gitlab-ee/issues/4539) in [GitLab Ultimate][ee] 10.4. > [Brought to GitLab Core](https://gitlab.com/gitlab-org/gitlab-ce/issues/44157) in 10.7. The Web IDE makes it faster and easier to contribute changes to your projects @@ -15,7 +15,7 @@ and from merge requests. ## File finder -> [Introduced in](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/18323) [GitLab Core][ce] 10.8. +> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/18323) in [GitLab Core][ce] 10.8. The file finder allows you to quickly open files in the current branch by searching. The file finder is launched using the keyboard shortcut `Command-p`, @@ -65,7 +65,7 @@ shows you a preview of the merge request diff if you commit your changes. ## View CI job logs -> [Introduced in](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/19279) [GitLab Core][ce] 11.0. +> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/19279) in [GitLab Core][ce] 11.0. The Web IDE can be used to quickly fix failing tests by opening the branch or merge request in the Web IDE and opening the logs of the failed job. The status @@ -77,7 +77,7 @@ left. ## Switching merge requests -> [Introduced in](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/19318) [GitLab Core][ce] 11.0. +> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/19318) in [GitLab Core][ce] 11.0. Switching between your authored and assigned merge requests can be done without leaving the Web IDE. Click the dropdown in the top of the sidebar to open a list @@ -86,7 +86,7 @@ switching to a different merge request. ## Switching branches -> [Introduced in](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/20850) [GitLab Core][ce] 11.2. +> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/20850) in [GitLab Core][ce] 11.2. Switching between branches of the current project repository can be done without leaving the Web IDE. Click the dropdown in the top of the sidebar to open a list @@ -95,7 +95,7 @@ switching to a different branch. ## Client Side Evaluation -> [Introduced in](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/19764) [GitLab Core][ce] 11.2. +> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/19764) in [GitLab Core][ce] 11.2. The Web IDE can be used to preview JavaScript projects right in the browser. This feature uses CodeSandbox to compile and bundle the JavaScript used to -- GitLab From 2d123a5075af2f73a1a4ea7ae60bea70e468e522 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 12 Dec 2018 12:57:42 +0000 Subject: [PATCH 05/47] Merge branch 'issue_55133' into 'master' Fix issuables sort direction button parameters Closes #55133 See merge request gitlab-org/gitlab-ce!23744 (cherry picked from commit 4dfe717848c4541b32cec887a1386fbc88ee0633) 61ad1da1 Fix issuables sort direction button parameters --- app/helpers/sorting_helper.rb | 2 +- spec/helpers/sorting_helper_spec.rb | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/app/helpers/sorting_helper.rb b/app/helpers/sorting_helper.rb index f51b96ba8ce..67c808b167a 100644 --- a/app/helpers/sorting_helper.rb +++ b/app/helpers/sorting_helper.rb @@ -164,7 +164,7 @@ module SortingHelper reverse_sort = issuable_reverse_sort_order_hash[sort_value] if reverse_sort - reverse_url = page_filter_path(sort: reverse_sort) + reverse_url = page_filter_path(sort: reverse_sort, label: true) else reverse_url = '#' link_class += ' disabled' diff --git a/spec/helpers/sorting_helper_spec.rb b/spec/helpers/sorting_helper_spec.rb index cba0d93e144..f405268d198 100644 --- a/spec/helpers/sorting_helper_spec.rb +++ b/spec/helpers/sorting_helper_spec.rb @@ -21,7 +21,11 @@ describe SortingHelper do describe '#issuable_sort_direction_button' do before do - allow(self).to receive(:request).and_return(double(path: 'http://test.com', query_parameters: {})) + allow(self).to receive(:request).and_return(double(path: 'http://test.com', query_parameters: { label_name: 'test_label' })) + end + + it 'keeps label filter param' do + expect(issuable_sort_direction_button('created_date')).to include('label_name=test_label') end it 'returns icon with sort-highest when sort is created_date' do -- GitLab From 8e347203657630643cf24bfc501b35f0f9276943 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Wed, 12 Dec 2018 14:20:27 +0000 Subject: [PATCH 06/47] Merge branch 'qa-mr-wait-for-push' into 'master' Wait for push before trying to create a new MR See merge request gitlab-org/gitlab-ce!23745 (cherry picked from commit c6db4471aa73e68e558c22b8009be826a267d565) 249a2c0e Wait for push before trying to create a new MR --- qa/qa/resource/merge_request.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/qa/qa/resource/merge_request.rb b/qa/qa/resource/merge_request.rb index 77afb3cfcba..cdfcf2b8742 100644 --- a/qa/qa/resource/merge_request.rb +++ b/qa/qa/resource/merge_request.rb @@ -58,7 +58,10 @@ module QA populate(:target, :source) project.visit! - Page::Project::Show.perform(&:new_merge_request) + Page::Project::Show.perform do |project| + project.wait_for_push + project.new_merge_request + end Page::MergeRequest::New.perform do |page| page.fill_title(@title) page.fill_description(@description) -- GitLab From e65afff444af6af3584f84684e0f54d6ca162c8d Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 13 Dec 2018 10:39:55 +0000 Subject: [PATCH 07/47] Merge branch 're-define-default-only-except-policy' into 'master' Re-define default only except policy Closes #55099 See merge request gitlab-org/gitlab-ce!23765 (cherry picked from commit 42f45ed2d93baa5b2b2f2c51f5bd8527acf1df95) 7381f6a8 Revert "Define the default value for only/except policies" 74443274 Re-define a way how we define default only/except policy --- ...ine-default-value-for-only-except-keys.yml | 2 +- lib/gitlab/ci/config/entry/except_policy.rb | 17 -- lib/gitlab/ci/config/entry/job.rb | 15 +- lib/gitlab/ci/config/entry/only_policy.rb | 18 -- lib/gitlab/ci/config/entry/policy.rb | 15 +- .../ci/config/entry/except_policy_spec.rb | 15 -- .../lib/gitlab/ci/config/entry/global_spec.rb | 6 +- spec/lib/gitlab/ci/config/entry/job_spec.rb | 3 +- spec/lib/gitlab/ci/config/entry/jobs_spec.rb | 6 +- .../ci/config/entry/only_policy_spec.rb | 15 -- .../lib/gitlab/ci/config/entry/policy_spec.rb | 167 +++++++++++++++++- .../ci/create_pipeline_service_spec.rb | 31 ++++ .../only_except_policy_examples.rb | 167 ------------------ 13 files changed, 223 insertions(+), 254 deletions(-) delete mode 100644 lib/gitlab/ci/config/entry/except_policy.rb delete mode 100644 lib/gitlab/ci/config/entry/only_policy.rb delete mode 100644 spec/lib/gitlab/ci/config/entry/except_policy_spec.rb delete mode 100644 spec/lib/gitlab/ci/config/entry/only_policy_spec.rb delete mode 100644 spec/support/shared_examples/only_except_policy_examples.rb diff --git a/changelogs/unreleased/define-default-value-for-only-except-keys.yml b/changelogs/unreleased/define-default-value-for-only-except-keys.yml index 3e5ecdcf51e..ed0e982f0fc 100644 --- a/changelogs/unreleased/define-default-value-for-only-except-keys.yml +++ b/changelogs/unreleased/define-default-value-for-only-except-keys.yml @@ -1,5 +1,5 @@ --- title: Define the default value for only/except policies -merge_request: 23531 +merge_request: 23765 author: type: changed diff --git a/lib/gitlab/ci/config/entry/except_policy.rb b/lib/gitlab/ci/config/entry/except_policy.rb deleted file mode 100644 index 46ded35325d..00000000000 --- a/lib/gitlab/ci/config/entry/except_policy.rb +++ /dev/null @@ -1,17 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Ci - class Config - module Entry - ## - # Entry that represents an only/except trigger policy for the job. - # - class ExceptPolicy < Policy - def self.default - end - end - end - end - end -end diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb index 085be5da08d..50942fbdb40 100644 --- a/lib/gitlab/ci/config/entry/job.rb +++ b/lib/gitlab/ci/config/entry/job.rb @@ -16,6 +16,13 @@ module Gitlab dependencies before_script after_script variables environment coverage retry parallel extends].freeze + DEFAULT_ONLY_POLICY = { + refs: %w(branches tags) + }.freeze + + DEFAULT_EXCEPT_POLICY = { + }.freeze + validations do validates :config, allowed_keys: ALLOWED_KEYS validates :config, presence: true @@ -65,10 +72,10 @@ module Gitlab entry :services, Entry::Services, description: 'Services that will be used to execute this job.' - entry :only, Entry::OnlyPolicy, + entry :only, Entry::Policy, description: 'Refs policy this job will be executed for.' - entry :except, Entry::ExceptPolicy, + entry :except, Entry::Policy, description: 'Refs policy this job will be executed for.' entry :variables, Entry::Variables, @@ -154,8 +161,8 @@ module Gitlab services: services_value, stage: stage_value, cache: cache_value, - only: only_value, - except: except_value, + only: DEFAULT_ONLY_POLICY.deep_merge(only_value.to_h), + except: DEFAULT_EXCEPT_POLICY.deep_merge(except_value.to_h), variables: variables_defined? ? variables_value : nil, environment: environment_defined? ? environment_value : nil, environment_name: environment_defined? ? environment_value[:name] : nil, diff --git a/lib/gitlab/ci/config/entry/only_policy.rb b/lib/gitlab/ci/config/entry/only_policy.rb deleted file mode 100644 index 9a581b8e97e..00000000000 --- a/lib/gitlab/ci/config/entry/only_policy.rb +++ /dev/null @@ -1,18 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module Ci - class Config - module Entry - ## - # Entry that represents an only/except trigger policy for the job. - # - class OnlyPolicy < Policy - def self.default - { refs: %w[branches tags] } - end - end - end - end - end -end diff --git a/lib/gitlab/ci/config/entry/policy.rb b/lib/gitlab/ci/config/entry/policy.rb index 81e74a639fc..998da1f6837 100644 --- a/lib/gitlab/ci/config/entry/policy.rb +++ b/lib/gitlab/ci/config/entry/policy.rb @@ -5,9 +5,12 @@ module Gitlab class Config module Entry ## - # Base class for OnlyPolicy and ExceptPolicy + # Entry that represents an only/except trigger policy for the job. # class Policy < ::Gitlab::Config::Entry::Simplifiable + strategy :RefsPolicy, if: -> (config) { config.is_a?(Array) } + strategy :ComplexPolicy, if: -> (config) { config.is_a?(Hash) } + class RefsPolicy < ::Gitlab::Config::Entry::Node include ::Gitlab::Config::Entry::Validatable @@ -63,16 +66,6 @@ module Gitlab def self.default end - - ## - # Class-level execution won't be inherited by subclasses by default. - # Therefore, we need to explicitly execute that for OnlyPolicy and ExceptPolicy - def self.inherited(klass) - super - - klass.strategy :RefsPolicy, if: -> (config) { config.is_a?(Array) } - klass.strategy :ComplexPolicy, if: -> (config) { config.is_a?(Hash) } - end end end end diff --git a/spec/lib/gitlab/ci/config/entry/except_policy_spec.rb b/spec/lib/gitlab/ci/config/entry/except_policy_spec.rb deleted file mode 100644 index d036bf2f4d1..00000000000 --- a/spec/lib/gitlab/ci/config/entry/except_policy_spec.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Gitlab::Ci::Config::Entry::ExceptPolicy do - let(:entry) { described_class.new(config) } - - it_behaves_like 'correct only except policy' - - describe '.default' do - it 'does not have a default value' do - expect(described_class.default).to be_nil - end - end -end diff --git a/spec/lib/gitlab/ci/config/entry/global_spec.rb b/spec/lib/gitlab/ci/config/entry/global_spec.rb index 12f4b9dc624..61d78f86b51 100644 --- a/spec/lib/gitlab/ci/config/entry/global_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/global_spec.rb @@ -161,7 +161,8 @@ describe Gitlab::Ci::Config::Entry::Global do variables: { 'VAR' => 'value' }, ignore: false, after_script: ['make clean'], - only: { refs: %w[branches tags] } }, + only: { refs: %w[branches tags] }, + except: {} }, spinach: { name: :spinach, before_script: [], script: %w[spinach], @@ -173,7 +174,8 @@ describe Gitlab::Ci::Config::Entry::Global do variables: {}, ignore: false, after_script: ['make clean'], - only: { refs: %w[branches tags] } } + only: { refs: %w[branches tags] }, + except: {} } ) end end diff --git a/spec/lib/gitlab/ci/config/entry/job_spec.rb b/spec/lib/gitlab/ci/config/entry/job_spec.rb index c1f4a060063..8e32cede3b5 100644 --- a/spec/lib/gitlab/ci/config/entry/job_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/job_spec.rb @@ -259,7 +259,8 @@ describe Gitlab::Ci::Config::Entry::Job do stage: 'test', ignore: false, after_script: %w[cleanup], - only: { refs: %w[branches tags] }) + only: { refs: %w[branches tags] }, + except: {}) end end end diff --git a/spec/lib/gitlab/ci/config/entry/jobs_spec.rb b/spec/lib/gitlab/ci/config/entry/jobs_spec.rb index 2a753408f54..1a2c30d3571 100644 --- a/spec/lib/gitlab/ci/config/entry/jobs_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/jobs_spec.rb @@ -68,13 +68,15 @@ describe Gitlab::Ci::Config::Entry::Jobs do commands: 'rspec', ignore: false, stage: 'test', - only: { refs: %w[branches tags] } }, + only: { refs: %w[branches tags] }, + except: {} }, spinach: { name: :spinach, script: %w[spinach], commands: 'spinach', ignore: false, stage: 'test', - only: { refs: %w[branches tags] } }) + only: { refs: %w[branches tags] }, + except: {} }) end end diff --git a/spec/lib/gitlab/ci/config/entry/only_policy_spec.rb b/spec/lib/gitlab/ci/config/entry/only_policy_spec.rb deleted file mode 100644 index 5518b68e51a..00000000000 --- a/spec/lib/gitlab/ci/config/entry/only_policy_spec.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Gitlab::Ci::Config::Entry::OnlyPolicy do - let(:entry) { described_class.new(config) } - - it_behaves_like 'correct only except policy' - - describe '.default' do - it 'haa a default value' do - expect(described_class.default).to eq( { refs: %w[branches tags] } ) - end - end -end diff --git a/spec/lib/gitlab/ci/config/entry/policy_spec.rb b/spec/lib/gitlab/ci/config/entry/policy_spec.rb index cf40a22af2e..83001b7fdd8 100644 --- a/spec/lib/gitlab/ci/config/entry/policy_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/policy_spec.rb @@ -1,8 +1,173 @@ -require 'spec_helper' +require 'fast_spec_helper' +require_dependency 'active_model' describe Gitlab::Ci::Config::Entry::Policy do let(:entry) { described_class.new(config) } + context 'when using simplified policy' do + describe 'validations' do + context 'when entry config value is valid' do + context 'when config is a branch or tag name' do + let(:config) { %w[master feature/branch] } + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + end + + describe '#value' do + it 'returns refs hash' do + expect(entry.value).to eq(refs: config) + end + end + end + + context 'when config is a regexp' do + let(:config) { ['/^issue-.*$/'] } + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + end + end + + context 'when config is a special keyword' do + let(:config) { %w[tags triggers branches] } + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + end + end + end + + context 'when entry value is not valid' do + let(:config) { [1] } + + describe '#errors' do + it 'saves errors' do + expect(entry.errors) + .to include /policy config should be an array of strings or regexps/ + end + end + end + end + end + + context 'when using complex policy' do + context 'when specifying refs policy' do + let(:config) { { refs: ['master'] } } + + it 'is a correct configuraton' do + expect(entry).to be_valid + expect(entry.value).to eq(refs: %w[master]) + end + end + + context 'when specifying kubernetes policy' do + let(:config) { { kubernetes: 'active' } } + + it 'is a correct configuraton' do + expect(entry).to be_valid + expect(entry.value).to eq(kubernetes: 'active') + end + end + + context 'when specifying invalid kubernetes policy' do + let(:config) { { kubernetes: 'something' } } + + it 'reports an error about invalid policy' do + expect(entry.errors).to include /unknown value: something/ + end + end + + context 'when specifying valid variables expressions policy' do + let(:config) { { variables: ['$VAR == null'] } } + + it 'is a correct configuraton' do + expect(entry).to be_valid + expect(entry.value).to eq(config) + end + end + + context 'when specifying variables expressions in invalid format' do + let(:config) { { variables: '$MY_VAR' } } + + it 'reports an error about invalid format' do + expect(entry.errors).to include /should be an array of strings/ + end + end + + context 'when specifying invalid variables expressions statement' do + let(:config) { { variables: ['$MY_VAR =='] } } + + it 'reports an error about invalid statement' do + expect(entry.errors).to include /invalid expression syntax/ + end + end + + context 'when specifying invalid variables expressions token' do + let(:config) { { variables: ['$MY_VAR == 123'] } } + + it 'reports an error about invalid expression' do + expect(entry.errors).to include /invalid expression syntax/ + end + end + + context 'when using invalid variables expressions regexp' do + let(:config) { { variables: ['$MY_VAR =~ /some ( thing/'] } } + + it 'reports an error about invalid expression' do + expect(entry.errors).to include /invalid expression syntax/ + end + end + + context 'when specifying a valid changes policy' do + let(:config) { { changes: %w[some/* paths/**/*.rb] } } + + it 'is a correct configuraton' do + expect(entry).to be_valid + expect(entry.value).to eq(config) + end + end + + context 'when changes policy is invalid' do + let(:config) { { changes: [1, 2] } } + + it 'returns errors' do + expect(entry.errors).to include /changes should be an array of strings/ + end + end + + context 'when specifying unknown policy' do + let(:config) { { refs: ['master'], invalid: :something } } + + it 'returns error about invalid key' do + expect(entry.errors).to include /unknown keys: invalid/ + end + end + + context 'when policy is empty' do + let(:config) { {} } + + it 'is not a valid configuration' do + expect(entry.errors).to include /can't be blank/ + end + end + end + + context 'when policy strategy does not match' do + let(:config) { 'string strategy' } + + it 'returns information about errors' do + expect(entry.errors) + .to include /has to be either an array of conditions or a hash/ + end + end + describe '.default' do it 'does not have a default value' do expect(described_class.default).to be_nil diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 8b8021ecbc8..ffa47d527f7 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -840,6 +840,37 @@ describe Ci::CreatePipelineService do end end + context "when config uses variables for only keyword" do + let(:config) do + { + build: { + stage: 'build', + script: 'echo', + only: { + variables: %w($CI) + } + } + } + end + + context 'when merge request is specified' do + let(:merge_request) do + create(:merge_request, + source_project: project, + source_branch: ref_name, + target_project: project, + target_branch: 'master') + end + + it 'does not create a merge request pipeline' do + expect(pipeline).not_to be_persisted + + expect(pipeline.errors[:base]) + .to eq(['No stages / jobs for this pipeline.']) + end + end + end + context "when config has 'except: [tags]'" do let(:config) do { diff --git a/spec/support/shared_examples/only_except_policy_examples.rb b/spec/support/shared_examples/only_except_policy_examples.rb deleted file mode 100644 index 35240af1d74..00000000000 --- a/spec/support/shared_examples/only_except_policy_examples.rb +++ /dev/null @@ -1,167 +0,0 @@ -# frozen_string_literal: true - -shared_examples 'correct only except policy' do - context 'when using simplified policy' do - describe 'validations' do - context 'when entry config value is valid' do - context 'when config is a branch or tag name' do - let(:config) { %w[master feature/branch] } - - describe '#valid?' do - it 'is valid' do - expect(entry).to be_valid - end - end - - describe '#value' do - it 'returns refs hash' do - expect(entry.value).to eq(refs: config) - end - end - end - - context 'when config is a regexp' do - let(:config) { ['/^issue-.*$/'] } - - describe '#valid?' do - it 'is valid' do - expect(entry).to be_valid - end - end - end - - context 'when config is a special keyword' do - let(:config) { %w[tags triggers branches] } - - describe '#valid?' do - it 'is valid' do - expect(entry).to be_valid - end - end - end - end - - context 'when entry value is not valid' do - let(:config) { [1] } - - describe '#errors' do - it 'saves errors' do - expect(entry.errors) - .to include /policy config should be an array of strings or regexps/ - end - end - end - end - end - - context 'when using complex policy' do - context 'when specifying refs policy' do - let(:config) { { refs: ['master'] } } - - it 'is a correct configuraton' do - expect(entry).to be_valid - expect(entry.value).to eq(refs: %w[master]) - end - end - - context 'when specifying kubernetes policy' do - let(:config) { { kubernetes: 'active' } } - - it 'is a correct configuraton' do - expect(entry).to be_valid - expect(entry.value).to eq(kubernetes: 'active') - end - end - - context 'when specifying invalid kubernetes policy' do - let(:config) { { kubernetes: 'something' } } - - it 'reports an error about invalid policy' do - expect(entry.errors).to include /unknown value: something/ - end - end - - context 'when specifying valid variables expressions policy' do - let(:config) { { variables: ['$VAR == null'] } } - - it 'is a correct configuraton' do - expect(entry).to be_valid - expect(entry.value).to eq(config) - end - end - - context 'when specifying variables expressions in invalid format' do - let(:config) { { variables: '$MY_VAR' } } - - it 'reports an error about invalid format' do - expect(entry.errors).to include /should be an array of strings/ - end - end - - context 'when specifying invalid variables expressions statement' do - let(:config) { { variables: ['$MY_VAR =='] } } - - it 'reports an error about invalid statement' do - expect(entry.errors).to include /invalid expression syntax/ - end - end - - context 'when specifying invalid variables expressions token' do - let(:config) { { variables: ['$MY_VAR == 123'] } } - - it 'reports an error about invalid expression' do - expect(entry.errors).to include /invalid expression syntax/ - end - end - - context 'when using invalid variables expressions regexp' do - let(:config) { { variables: ['$MY_VAR =~ /some ( thing/'] } } - - it 'reports an error about invalid expression' do - expect(entry.errors).to include /invalid expression syntax/ - end - end - - context 'when specifying a valid changes policy' do - let(:config) { { changes: %w[some/* paths/**/*.rb] } } - - it 'is a correct configuraton' do - expect(entry).to be_valid - expect(entry.value).to eq(config) - end - end - - context 'when changes policy is invalid' do - let(:config) { { changes: [1, 2] } } - - it 'returns errors' do - expect(entry.errors).to include /changes should be an array of strings/ - end - end - - context 'when specifying unknown policy' do - let(:config) { { refs: ['master'], invalid: :something } } - - it 'returns error about invalid key' do - expect(entry.errors).to include /unknown keys: invalid/ - end - end - - context 'when policy is empty' do - let(:config) { {} } - - it 'is not a valid configuration' do - expect(entry.errors).to include /can't be blank/ - end - end - end - - context 'when policy strategy does not match' do - let(:config) { 'string strategy' } - - it 'returns information about errors' do - expect(entry.errors) - .to include /has to be either an array of conditions or a hash/ - end - end -end -- GitLab From 946fea7416b930e41894f4533f71b607f99a7092 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 13 Dec 2018 09:55:09 +0000 Subject: [PATCH 08/47] Merge branch 'fixed-diff-notes-not-showing-ce' into 'master' Fixed notes not being applied to diff lines Closes #55245 and #55248 See merge request gitlab-org/gitlab-ce!23778 (cherry picked from commit 29731a0321bbbc0f3305579ec32d9ca4a00e5fbd) 210e90df Fixed notes not being applied to diff lines efbb9501 Added test 7a5c9f8a Fixed parallel discussions throwing an error --- .../javascripts/diffs/store/mutations.js | 45 ++++------ .../javascripts/diffs/store/mutations_spec.js | 85 +++++++++++++++++++ 2 files changed, 102 insertions(+), 28 deletions(-) diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 331fb052371..816dcee4e00 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -123,22 +123,23 @@ export default { diffPosition: diffPositionByLineCode[line.line_code], latestDiff, }); + const mapDiscussions = (line, extraCheck = () => true) => ({ + ...line, + discussions: extraCheck() + ? line.discussions + .filter(() => !line.discussions.some(({ id }) => discussion.id === id)) + .concat(lineCheck(line) ? discussion : line.discussions) + : [], + }); state.diffFiles = state.diffFiles.map(diffFile => { if (diffFile.file_hash === fileHash) { const file = { ...diffFile }; if (file.highlighted_diff_lines) { - file.highlighted_diff_lines = file.highlighted_diff_lines.map(line => { - if (!line.discussions.some(({ id }) => discussion.id === id) && lineCheck(line)) { - return { - ...line, - discussions: line.discussions.concat(discussion), - }; - } - - return line; - }); + file.highlighted_diff_lines = file.highlighted_diff_lines.map(line => + mapDiscussions(line), + ); } if (file.parallel_diff_lines) { @@ -148,20 +149,8 @@ export default { if (left || right) { return { - left: { - ...line.left, - discussions: - left && !line.left.discussions.some(({ id }) => id === discussion.id) - ? line.left.discussions.concat(discussion) - : (line.left && line.left.discussions) || [], - }, - right: { - ...line.right, - discussions: - right && !left && !line.right.discussions.some(({ id }) => id === discussion.id) - ? line.right.discussions.concat(discussion) - : (line.right && line.right.discussions) || [], - }, + left: line.left ? mapDiscussions(line.left) : null, + right: line.right ? mapDiscussions(line.right, () => !left) : null, }; } @@ -180,7 +169,7 @@ export default { }); }, - [types.REMOVE_LINE_DISCUSSIONS_FOR_FILE](state, { fileHash, lineCode, id }) { + [types.REMOVE_LINE_DISCUSSIONS_FOR_FILE](state, { fileHash, lineCode }) { const selectedFile = state.diffFiles.find(f => f.file_hash === fileHash); if (selectedFile) { if (selectedFile.parallel_diff_lines) { @@ -193,7 +182,7 @@ export default { const side = targetLine.left && targetLine.left.line_code === lineCode ? 'left' : 'right'; Object.assign(targetLine[side], { - discussions: [], + discussions: targetLine[side].discussions.filter(discussion => discussion.notes.length), }); } } @@ -205,14 +194,14 @@ export default { if (targetInlineLine) { Object.assign(targetInlineLine, { - discussions: [], + discussions: targetInlineLine.discussions.filter(discussion => discussion.notes.length), }); } } if (selectedFile.discussions && selectedFile.discussions.length) { selectedFile.discussions = selectedFile.discussions.filter( - discussion => discussion.id !== id, + discussion => discussion.notes.length, ); } } diff --git a/spec/javascripts/diffs/store/mutations_spec.js b/spec/javascripts/diffs/store/mutations_spec.js index 23e8761bc55..f3449bec6ec 100644 --- a/spec/javascripts/diffs/store/mutations_spec.js +++ b/spec/javascripts/diffs/store/mutations_spec.js @@ -277,6 +277,87 @@ describe('DiffsStoreMutations', () => { expect(state.diffFiles[0].highlighted_diff_lines[0].discussions[0].id).toEqual(1); }); + it('updates existing discussion', () => { + const diffPosition = { + base_sha: 'ed13df29948c41ba367caa757ab3ec4892509910', + head_sha: 'b921914f9a834ac47e6fd9420f78db0f83559130', + new_line: null, + new_path: '500-lines-4.txt', + old_line: 5, + old_path: '500-lines-4.txt', + start_sha: 'ed13df29948c41ba367caa757ab3ec4892509910', + }; + + const state = { + latestDiff: true, + diffFiles: [ + { + file_hash: 'ABC', + parallel_diff_lines: [ + { + left: { + line_code: 'ABC_1', + discussions: [], + }, + right: { + line_code: 'ABC_1', + discussions: [], + }, + }, + ], + highlighted_diff_lines: [ + { + line_code: 'ABC_1', + discussions: [], + }, + ], + }, + ], + }; + const discussion = { + id: 1, + line_code: 'ABC_1', + diff_discussion: true, + resolvable: true, + original_position: diffPosition, + position: diffPosition, + diff_file: { + file_hash: state.diffFiles[0].file_hash, + }, + }; + + const diffPositionByLineCode = { + ABC_1: diffPosition, + }; + + mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { + discussion, + diffPositionByLineCode, + }); + + expect(state.diffFiles[0].parallel_diff_lines[0].left.discussions.length).toEqual(1); + expect(state.diffFiles[0].parallel_diff_lines[0].left.discussions[0].id).toEqual(1); + expect(state.diffFiles[0].parallel_diff_lines[0].right.discussions).toEqual([]); + + expect(state.diffFiles[0].highlighted_diff_lines[0].discussions.length).toEqual(1); + expect(state.diffFiles[0].highlighted_diff_lines[0].discussions[0].id).toEqual(1); + + mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { + discussion: { + ...discussion, + resolved: true, + notes: ['test'], + }, + diffPositionByLineCode, + }); + + expect(state.diffFiles[0].parallel_diff_lines[0].left.discussions[0].notes.length).toBe(1); + expect(state.diffFiles[0].highlighted_diff_lines[0].discussions[0].notes.length).toBe(1); + + expect(state.diffFiles[0].parallel_diff_lines[0].left.discussions[0].resolved).toBe(true); + expect(state.diffFiles[0].highlighted_diff_lines[0].discussions[0].resolved).toBe(true); + }); + it('should add legacy discussions to the given line', () => { const diffPosition = { base_sha: 'ed13df29948c41ba367caa757ab3ec4892509910', @@ -356,10 +437,12 @@ describe('DiffsStoreMutations', () => { { id: 1, line_code: 'ABC_1', + notes: [], }, { id: 2, line_code: 'ABC_1', + notes: [], }, ], }, @@ -376,10 +459,12 @@ describe('DiffsStoreMutations', () => { { id: 1, line_code: 'ABC_1', + notes: [], }, { id: 2, line_code: 'ABC_1', + notes: [], }, ], }, -- GitLab From 552bb530c0b35d2982903aa38c382afd38d0525b Mon Sep 17 00:00:00 2001 From: Fatih Acet Date: Thu, 13 Dec 2018 13:00:41 +0000 Subject: [PATCH 09/47] Merge branch '55257-fix-sm-button-sizes' into 'master' Fix small button line height Closes #55257 See merge request gitlab-org/gitlab-ce!23779 (cherry picked from commit c7ac2c715abf45846748b056239d2e6586ce29f7) 9c001c0d Fix small button line height --- app/assets/stylesheets/framework/buttons.scss | 4 ++-- app/assets/stylesheets/framework/variables.scss | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/assets/stylesheets/framework/buttons.scss b/app/assets/stylesheets/framework/buttons.scss index e36f99ac577..a4a9276c580 100644 --- a/app/assets/stylesheets/framework/buttons.scss +++ b/app/assets/stylesheets/framework/buttons.scss @@ -148,8 +148,8 @@ &.btn-xs { padding: 2px $gl-btn-padding; - font-size: $gl-btn-small-font-size; - line-height: $gl-btn-small-line-height; + font-size: $gl-btn-xs-font-size; + line-height: $gl-btn-xs-line-height; } &.btn-success, diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index 134b3a4521b..db9461cb001 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -368,7 +368,9 @@ $gl-btn-line-height: 16px; $gl-btn-vert-padding: 8px; $gl-btn-horz-padding: 12px; $gl-btn-small-font-size: 13px; -$gl-btn-small-line-height: 13px; +$gl-btn-small-line-height: 18px; +$gl-btn-xs-font-size: 13px; +$gl-btn-xs-line-height: 13px; /* * Badges -- GitLab From 0c0b849be1c4499d69bd6afa4be9ee74fa9c7a78 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 13 Dec 2018 08:37:59 +0000 Subject: [PATCH 10/47] Merge branch '55221-fix-mr-responsive-icons' into 'master' Fix mr_widget_icon responsive alignment See merge request gitlab-org/gitlab-ce!23781 (cherry picked from commit 0a6c217c743928378545c2477c0039ecae0005af) e5348394 Fix mr_widget_icon responsive alignment --- .../vue_merge_request_widget/components/mr_widget_icon.vue | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_icon.vue b/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_icon.vue index e3adc7f7af5..4b57693e8f1 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_icon.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_icon.vue @@ -13,5 +13,7 @@ export default { -- GitLab From 670796e15d52776aaee029ab4e108783d5974139 Mon Sep 17 00:00:00 2001 From: Fatih Acet Date: Fri, 14 Dec 2018 11:49:53 +0000 Subject: [PATCH 11/47] Merge branch 'winh-resolved-discussions-reply-field' into 'master' Display reply field if resolved discussion has no replies Closes #54330 See merge request gitlab-org/gitlab-ce!23801 (cherry picked from commit 1d0eadc470a2c7324d002e923eb76ae932862354) 06897b76 Add test for missing reply field on resolved discussions 518b2cdf Display reply field if resolved discussion has no replies --- .../notes/components/noteable_discussion.vue | 2 +- .../winh-resolved-discussions-reply-field.yml | 5 +++++ .../features/discussion_comments_shared_example.rb | 10 ++++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/winh-resolved-discussions-reply-field.yml diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue index f4991a41325..6503f236389 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -379,7 +379,7 @@ Please check your network connection and try again.`;
diff --git a/changelogs/unreleased/winh-resolved-discussions-reply-field.yml b/changelogs/unreleased/winh-resolved-discussions-reply-field.yml new file mode 100644 index 00000000000..01cf35ae8a7 --- /dev/null +++ b/changelogs/unreleased/winh-resolved-discussions-reply-field.yml @@ -0,0 +1,5 @@ +--- +title: Display reply field if resolved discussion has no replies +merge_request: 23801 +author: +type: fixed diff --git a/spec/support/features/discussion_comments_shared_example.rb b/spec/support/features/discussion_comments_shared_example.rb index 922f3df144d..42a086d58d2 100644 --- a/spec/support/features/discussion_comments_shared_example.rb +++ b/spec/support/features/discussion_comments_shared_example.rb @@ -178,6 +178,16 @@ shared_examples 'discussion comments' do |resource_name| let(:note_id) { find("#{comments_selector} .note:first-child", match: :first)['data-note-id'] } let(:reply_id) { find("#{comments_selector} .note:last-child", match: :first)['data-note-id'] } + it 'can be replied to after resolving' do + click_button "Resolve discussion" + wait_for_requests + + refresh + wait_for_requests + + submit_reply('to reply or not reply') + end + it 'shows resolved discussion when toggled' do submit_reply('a') -- GitLab From 73518d57bb46c0756fc2aa92bfdba3438425fdb6 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 14 Dec 2018 00:26:05 +0000 Subject: [PATCH 12/47] Merge branch 'bw-fix-missing-board-milestone' into 'master' Fix Issue boards don't show milestone in issue details See merge request gitlab-org/gitlab-ce!23813 (cherry picked from commit 0087396036b265879771380e60787e2072318fd1) fa7b4eca Use proper API::Entities::Milestone --- app/serializers/issue_board_entity.rb | 2 +- spec/serializers/issue_board_entity_spec.rb | 31 +++++++++++++++++---- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/app/serializers/issue_board_entity.rb b/app/serializers/issue_board_entity.rb index 58ab804a3c8..e3dc43240c6 100644 --- a/app/serializers/issue_board_entity.rb +++ b/app/serializers/issue_board_entity.rb @@ -17,7 +17,7 @@ class IssueBoardEntity < Grape::Entity end expose :milestone, expose_nil: false do |issue| - API::Entities::Project.represent issue.milestone, only: [:id, :title] + API::Entities::Milestone.represent issue.milestone, only: [:id, :title] end expose :assignees do |issue| diff --git a/spec/serializers/issue_board_entity_spec.rb b/spec/serializers/issue_board_entity_spec.rb index 06d9d3657e6..f6fa2a794f6 100644 --- a/spec/serializers/issue_board_entity_spec.rb +++ b/spec/serializers/issue_board_entity_spec.rb @@ -3,21 +3,40 @@ require 'spec_helper' describe IssueBoardEntity do - let(:project) { create(:project) } - let(:resource) { create(:issue, project: project) } - let(:user) { create(:user) } - - let(:request) { double('request', current_user: user) } + let(:project) { create(:project) } + let(:resource) { create(:issue, project: project) } + let(:user) { create(:user) } + let(:milestone) { create(:milestone, project: project) } + let(:label) { create(:label, project: project, title: 'Test Label') } + let(:request) { double('request', current_user: user) } subject { described_class.new(resource, request: request).as_json } it 'has basic attributes' do expect(subject).to include(:id, :iid, :title, :confidential, :due_date, :project_id, :relative_position, - :project, :labels) + :labels, :assignees, project: hash_including(:id, :path)) end it 'has path and endpoints' do expect(subject).to include(:reference_path, :real_path, :issue_sidebar_endpoint, :toggle_subscription_endpoint, :assignable_labels_endpoint) end + + it 'has milestone attributes' do + resource.milestone = milestone + + expect(subject).to include(milestone: hash_including(:id, :title)) + end + + it 'has assignee attributes' do + resource.assignees = [user] + + expect(subject).to include(assignees: array_including(hash_including(:id, :name, :username, :avatar_url))) + end + + it 'has label attributes' do + resource.labels = [label] + + expect(subject).to include(labels: array_including(hash_including(:id, :title, :color, :description, :text_color, :priority))) + end end -- GitLab From f151556e331d46a2bc190a5fbeaf5e967eb8397b Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 13 Dec 2018 19:17:19 +0000 Subject: [PATCH 13/47] Merge branch 'osw-suggest-diff-line-change' into 'master' Allow suggesting single line changes in diffs See merge request gitlab-org/gitlab-ce!23147 --- app/assets/javascripts/api.js | 7 + .../javascripts/diffs/components/app.vue | 11 + .../diffs/components/diff_content.vue | 7 + .../diffs/components/diff_discussions.vue | 12 + .../diffs/components/diff_file.vue | 6 + .../diffs/components/diff_line_note_form.vue | 1 + .../components/inline_diff_comment_row.vue | 12 +- .../diffs/components/inline_diff_view.vue | 6 + .../components/parallel_diff_comment_row.vue | 9 + .../diffs/components/parallel_diff_view.vue | 6 + app/assets/javascripts/diffs/index.js | 2 + .../javascripts/lib/utils/text_markdown.js | 42 +++- app/assets/javascripts/mr_notes/index.js | 2 + .../notes/components/note_body.vue | 38 ++- .../notes/components/note_form.vue | 35 ++- .../notes/components/noteable_discussion.vue | 24 ++ .../notes/components/noteable_note.vue | 12 + .../notes/components/notes_app.vue | 6 + .../notes/services/notes_service.js | 4 + .../javascripts/notes/stores/actions.js | 20 ++ .../javascripts/notes/stores/modules/index.js | 1 + .../notes/stores/mutation_types.js | 1 + .../javascripts/notes/stores/mutations.js | 11 + .../vue_shared/components/markdown/field.vue | 97 +++++++- .../vue_shared/components/markdown/header.vue | 23 ++ .../components/markdown/suggestion_diff.vue | 74 ++++++ .../markdown/suggestion_diff_header.vue | 60 +++++ .../components/markdown/suggestions.vue | 136 +++++++++++ .../components/markdown/toolbar_button.vue | 12 + .../stylesheets/framework/markdown_area.scss | 21 ++ .../stylesheets/framework/variables.scss | 1 + app/controllers/concerns/preview_markdown.rb | 10 +- app/models/concerns/noteable.rb | 4 + app/models/diff_note.rb | 13 + app/models/merge_request.rb | 5 + app/models/note.rb | 12 +- app/models/suggestion.rb | 61 +++++ app/policies/suggestion_policy.rb | 11 + app/serializers/diff_line_entity.rb | 2 + .../merge_request_widget_entity.rb | 2 + app/serializers/note_entity.rb | 1 + app/serializers/suggestion_entity.rb | 17 ++ app/services/notes/create_service.rb | 1 + app/services/notes/update_service.rb | 11 + app/services/preview_markdown_service.rb | 8 + app/services/suggestions/apply_service.rb | 54 +++++ app/services/suggestions/create_service.rb | 56 +++++ .../projects/merge_requests/show.html.haml | 2 + .../suggest-change-to-diff-line.yml | 5 + .../20181123144235_create_suggestions.rb | 20 ++ db/schema.rb | 11 + lib/api/api.rb | 1 + lib/api/entities.rb | 12 + lib/api/suggestions.rb | 31 +++ lib/banzai/filter/suggestion_filter.rb | 25 ++ lib/banzai/filter/syntax_highlight_filter.rb | 2 +- lib/banzai/pipeline/gfm_pipeline.rb | 1 + lib/banzai/pipeline/post_process_pipeline.rb | 3 +- lib/banzai/suggestions_parser.rb | 14 ++ lib/gitlab/diff/file.rb | 10 + lib/gitlab/diff/line.rb | 4 + lib/gitlab/usage_data.rb | 2 + locale/gitlab.pot | 15 ++ spec/db/schema_spec.rb | 3 +- spec/factories/suggestions.rb | 20 ++ .../user_suggests_changes_on_diff_spec.rb | 85 +++++++ .../api/schemas/entities/diff_line.json | 3 +- .../entities/merge_request_widget.json | 3 +- .../diffs/components/diff_content_spec.js | 1 + .../diffs/mock_data/diff_discussions.js | 15 +- .../components/markdown/header_spec.js | 15 ++ .../markdown/suggestion_diff_header_spec.js | 69 ++++++ .../markdown/suggestion_diff_spec.js | 79 ++++++ .../components/markdown/suggestions_spec.js | 125 ++++++++++ .../banzai/filter/suggestion_filter_spec.rb | 35 +++ spec/lib/banzai/suggestions_parser_spec.rb | 32 +++ spec/lib/gitlab/import_export/all_models.yml | 1 + spec/lib/gitlab/usage_data_spec.rb | 1 + spec/models/diff_note_spec.rb | 18 ++ spec/models/suggestion_spec.rb | 57 +++++ spec/requests/api/suggestions_spec.rb | 83 +++++++ spec/serializers/suggestion_entity_spec.rb | 23 ++ spec/services/notes/update_service_spec.rb | 23 ++ .../services/preview_markdown_service_spec.rb | 25 ++ .../suggestions/apply_service_spec.rb | 229 ++++++++++++++++++ .../suggestions/create_service_spec.rb | 110 +++++++++ 86 files changed, 2155 insertions(+), 25 deletions(-) create mode 100644 app/assets/javascripts/vue_shared/components/markdown/suggestion_diff.vue create mode 100644 app/assets/javascripts/vue_shared/components/markdown/suggestion_diff_header.vue create mode 100644 app/assets/javascripts/vue_shared/components/markdown/suggestions.vue create mode 100644 app/models/suggestion.rb create mode 100644 app/policies/suggestion_policy.rb create mode 100644 app/serializers/suggestion_entity.rb create mode 100644 app/services/suggestions/apply_service.rb create mode 100644 app/services/suggestions/create_service.rb create mode 100644 changelogs/unreleased/suggest-change-to-diff-line.yml create mode 100644 db/migrate/20181123144235_create_suggestions.rb create mode 100644 lib/api/suggestions.rb create mode 100644 lib/banzai/filter/suggestion_filter.rb create mode 100644 lib/banzai/suggestions_parser.rb create mode 100644 spec/factories/suggestions.rb create mode 100644 spec/features/merge_request/user_suggests_changes_on_diff_spec.rb create mode 100644 spec/javascripts/vue_shared/components/markdown/suggestion_diff_header_spec.js create mode 100644 spec/javascripts/vue_shared/components/markdown/suggestion_diff_spec.js create mode 100644 spec/javascripts/vue_shared/components/markdown/suggestions_spec.js create mode 100644 spec/lib/banzai/filter/suggestion_filter_spec.rb create mode 100644 spec/lib/banzai/suggestions_parser_spec.rb create mode 100644 spec/models/suggestion_spec.rb create mode 100644 spec/requests/api/suggestions_spec.rb create mode 100644 spec/serializers/suggestion_entity_spec.rb create mode 100644 spec/services/suggestions/apply_service_spec.rb create mode 100644 spec/services/suggestions/create_service_spec.rb diff --git a/app/assets/javascripts/api.js b/app/assets/javascripts/api.js index de003e70e61..e5a628ab653 100644 --- a/app/assets/javascripts/api.js +++ b/app/assets/javascripts/api.js @@ -23,6 +23,7 @@ const Api = { usersPath: '/api/:version/users.json', userStatusPath: '/api/:version/user/status', commitPath: '/api/:version/projects/:id/repository/commits', + applySuggestionPath: '/api/:version/suggestions/:id/apply', commitPipelinesPath: '/:project_id/commit/:sha/pipelines', branchSinglePath: '/api/:version/projects/:id/repository/branches/:branch', createBranchPath: '/api/:version/projects/:id/repository/branches', @@ -183,6 +184,12 @@ const Api = { }); }, + applySuggestion(id) { + const url = Api.buildUrl(Api.applySuggestionPath).replace(':id', encodeURIComponent(id)); + + return axios.put(url); + }, + commitPipelines(projectId, sha) { const encodedProjectId = projectId .split('/') diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index bf9244df7f7..7e5aca88984 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -42,6 +42,16 @@ export default { type: Object, required: true, }, + helpPagePath: { + type: String, + required: false, + default: '', + }, + changesEmptyStateIllustration: { + type: String, + required: false, + default: '', + }, }, data() { return { @@ -196,6 +206,7 @@ export default { v-for="file in diffFiles" :key="file.newPath" :file="file" + :help-page-path="helpPagePath" :can-current-user-fork="canCurrentUserFork" /> diff --git a/app/assets/javascripts/diffs/components/diff_content.vue b/app/assets/javascripts/diffs/components/diff_content.vue index 11cc4c09fed..ac963f2971e 100644 --- a/app/assets/javascripts/diffs/components/diff_content.vue +++ b/app/assets/javascripts/diffs/components/diff_content.vue @@ -23,6 +23,11 @@ export default { type: Object, required: true, }, + helpPagePath: { + type: String, + required: false, + default: '', + }, }, computed: { ...mapState({ @@ -74,11 +79,13 @@ export default { v-if="isInlineView" :diff-file="diffFile" :diff-lines="diffFile.highlighted_diff_lines || []" + :help-page-path="helpPagePath" /> diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue index 3b2a0d156ca..a2b6caaa346 100644 --- a/app/assets/javascripts/diffs/components/diff_file.vue +++ b/app/assets/javascripts/diffs/components/diff_file.vue @@ -22,6 +22,11 @@ export default { type: Boolean, required: true, }, + helpPagePath: { + type: String, + required: false, + default: '', + }, }, data() { return { @@ -160,6 +165,7 @@ export default { v-if="!isCollapsed && file.renderIt" :class="{ hidden: isCollapsed || file.too_large }" :diff-file="file" + :help-page-path="helpPagePath" />
diff --git a/app/assets/javascripts/diffs/components/diff_line_note_form.vue b/app/assets/javascripts/diffs/components/diff_line_note_form.vue index 9fd02acbd6e..e7569ba7b84 100644 --- a/app/assets/javascripts/diffs/components/diff_line_note_form.vue +++ b/app/assets/javascripts/diffs/components/diff_line_note_form.vue @@ -94,6 +94,7 @@ export default { ref="noteForm" :is-editing="true" :line-code="line.line_code" + :line="line" save-button-title="Comment" class="diff-comment-form" @cancelForm="handleCancelCommentForm" diff --git a/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue b/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue index aa40b24950a..814ee0b7c02 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue @@ -16,6 +16,11 @@ export default { type: String, required: true, }, + helpPagePath: { + type: String, + required: false, + default: '', + }, }, computed: { className() { @@ -38,7 +43,12 @@ export default {
- + diff --git a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue index b98463d3dd3..a65cf025cde 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue @@ -20,6 +20,11 @@ export default { type: Number, required: true, }, + helpPagePath: { + type: String, + required: false, + default: '', + }, }, computed: { hasExpandedDiscussionOnLeft() { @@ -87,6 +92,8 @@ export default {
diff --git a/app/assets/javascripts/diffs/index.js b/app/assets/javascripts/diffs/index.js index 06ef4207d85..41e6bd81072 100644 --- a/app/assets/javascripts/diffs/index.js +++ b/app/assets/javascripts/diffs/index.js @@ -16,6 +16,7 @@ export default function initDiffsApp(store) { return { endpoint: dataset.endpoint, projectPath: dataset.projectPath, + helpPagePath: dataset.helpPagePath, currentUser: JSON.parse(dataset.currentUserData) || {}, }; }, @@ -30,6 +31,7 @@ export default function initDiffsApp(store) { endpoint: this.endpoint, currentUser: this.currentUser, projectPath: this.projectPath, + helpPagePath: this.helpPagePath, shouldShow: this.activeTab === 'diffs', }, }); diff --git a/app/assets/javascripts/lib/utils/text_markdown.js b/app/assets/javascripts/lib/utils/text_markdown.js index 3618c6af7e2..c095a017866 100644 --- a/app/assets/javascripts/lib/utils/text_markdown.js +++ b/app/assets/javascripts/lib/utils/text_markdown.js @@ -39,7 +39,14 @@ function blockTagText(text, textArea, blockTag, selected) { } } -function moveCursor({ textArea, tag, positionBetweenTags, removedLastNewLine, select }) { +function moveCursor({ + textArea, + tag, + cursorOffset, + positionBetweenTags, + removedLastNewLine, + select, +}) { var pos; if (!textArea.setSelectionRange) { return; @@ -61,11 +68,24 @@ function moveCursor({ textArea, tag, positionBetweenTags, removedLastNewLine, se pos -= 1; } + if (cursorOffset) { + pos -= cursorOffset; + } + return textArea.setSelectionRange(pos, pos); } } -export function insertMarkdownText({ textArea, text, tag, blockTag, selected, wrap, select }) { +export function insertMarkdownText({ + textArea, + text, + tag, + cursorOffset, + blockTag, + selected, + wrap, + select, +}) { var textToInsert, selectedSplit, startChar, @@ -154,20 +174,30 @@ export function insertMarkdownText({ textArea, text, tag, blockTag, selected, wr return moveCursor({ textArea, tag: tag.replace(textPlaceholder, selected), + cursorOffset, positionBetweenTags: wrap && selected.length === 0, removedLastNewLine, select, }); } -function updateText({ textArea, tag, blockTag, wrap, select }) { +function updateText({ textArea, tag, cursorOffset, blockTag, wrap, select, tagContent }) { var $textArea, selected, text; $textArea = $(textArea); textArea = $textArea.get(0); text = $textArea.val(); - selected = selectedText(text, textArea); + selected = selectedText(text, textArea) || tagContent; $textArea.focus(); - return insertMarkdownText({ textArea, text, tag, blockTag, selected, wrap, select }); + return insertMarkdownText({ + textArea, + text, + tag, + cursorOffset, + blockTag, + selected, + wrap, + select, + }); } export function addMarkdownListeners(form) { @@ -178,9 +208,11 @@ export function addMarkdownListeners(form) { return updateText({ textArea: $this.closest('.md-area').find('textarea'), tag: $this.data('mdTag'), + cursorOffset: $this.data('mdCursorOffset'), blockTag: $this.data('mdBlock'), wrap: !$this.data('mdPrepend'), select: $this.data('mdSelect'), + tagContent: $this.data('mdTagContent').toString(), }); }); } diff --git a/app/assets/javascripts/mr_notes/index.js b/app/assets/javascripts/mr_notes/index.js index 1c98683c597..e4d72eb8318 100644 --- a/app/assets/javascripts/mr_notes/index.js +++ b/app/assets/javascripts/mr_notes/index.js @@ -33,6 +33,7 @@ export default function initMrNotes() { noteableData, currentUserData: JSON.parse(notesDataset.currentUserData), notesData: JSON.parse(notesDataset.notesData), + helpPagePath: notesDataset.helpPagePath, }; }, computed: { @@ -71,6 +72,7 @@ export default function initMrNotes() { notesData: this.notesData, userData: this.currentUserData, shouldShow: this.activeTab === 'show', + helpPagePath: this.helpPagePath, }, }); }, diff --git a/app/assets/javascripts/notes/components/note_body.vue b/app/assets/javascripts/notes/components/note_body.vue index c0bee600181..bcf5d334da4 100644 --- a/app/assets/javascripts/notes/components/note_body.vue +++ b/app/assets/javascripts/notes/components/note_body.vue @@ -1,10 +1,12 @@