diff options
| author | GitLab Bot <gitlab-bot@gitlab.com> | 2019-11-15 15:06:12 +0000 |
|---|---|---|
| committer | GitLab Bot <gitlab-bot@gitlab.com> | 2019-11-15 15:06:12 +0000 |
| commit | 6e81d7f6283fae1b22f66b9d9b133243921cbd9e (patch) | |
| tree | 8cf8052ef6734ceeb49314f15ff07d2720511f0d | |
| parent | 3fc9a8e6957ddf75576dc63069c4c0249514499f (diff) | |
| download | gitlab-ce-6e81d7f6283fae1b22f66b9d9b133243921cbd9e.tar.gz | |
Add latest changes from gitlab-org/gitlab@master
50 files changed, 577 insertions, 152 deletions
@@ -136,7 +136,7 @@ gem 'faraday_middleware-aws-signers-v4' # Markdown and HTML processing gem 'html-pipeline', '~> 2.8' -gem 'deckar01-task_list', '2.2.0' +gem 'deckar01-task_list', '2.2.1' gem 'gitlab-markup', '~> 1.7.0' gem 'github-markup', '~> 1.7.0', require: 'github/markup' gem 'commonmarker', '~> 0.17' diff --git a/Gemfile.lock b/Gemfile.lock index 5951eb4bf71..55fc902584c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -194,7 +194,7 @@ GEM database_cleaner (1.7.0) debug_inspector (0.0.3) debugger-ruby_core_source (1.3.8) - deckar01-task_list (2.2.0) + deckar01-task_list (2.2.1) html-pipeline declarative (0.0.10) declarative-option (0.1.0) @@ -604,7 +604,7 @@ GEM netrc (0.11.0) nio4r (2.3.1) no_proxy_fix (0.1.2) - nokogiri (1.10.4) + nokogiri (1.10.5) mini_portile2 (~> 2.4.0) nokogumbo (1.5.0) nokogiri @@ -1128,7 +1128,7 @@ DEPENDENCIES creole (~> 0.5.0) danger (~> 6.0) database_cleaner (~> 1.7.0) - deckar01-task_list (= 2.2.0) + deckar01-task_list (= 2.2.1) default_value_for (~> 3.3.0) derailed_benchmarks device_detector diff --git a/app/assets/javascripts/lib/utils/text_utility.js b/app/assets/javascripts/lib/utils/text_utility.js index b839a994455..0c194d67bce 100644 --- a/app/assets/javascripts/lib/utils/text_utility.js +++ b/app/assets/javascripts/lib/utils/text_utility.js @@ -36,25 +36,26 @@ export const humanize = string => export const dasherize = str => str.replace(/[_\s]+/g, '-'); /** - * Replaces whitespaces with hyphens, convert to lower case and remove non-allowed special characters - * @param {String} str + * Replaces whitespace and non-sluggish characters with a given separator + * @param {String} str - The string to slugify + * @param {String=} separator - The separator used to separate words (defaults to "-") * @returns {String} */ -export const slugify = str => { +export const slugify = (str, separator = '-') => { const slug = str .trim() .toLowerCase() - .replace(/[^a-zA-Z0-9_.-]+/g, '-'); + .replace(/[^a-zA-Z0-9_.-]+/g, separator); - return slug === '-' ? '' : slug; + return slug === separator ? '' : slug; }; /** - * Replaces whitespaces with underscore and converts to lower case + * Replaces whitespace and non-sluggish characters with underscores * @param {String} str * @returns {String} */ -export const slugifyWithUnderscore = str => str.toLowerCase().replace(/\s+/g, '_'); +export const slugifyWithUnderscore = str => slugify(str, '_'); /** * Truncates given text diff --git a/app/assets/javascripts/monitoring/components/date_time_picker/date_time_picker.vue b/app/assets/javascripts/monitoring/components/date_time_picker/date_time_picker.vue index 46d8f61cab6..8749019c5cd 100644 --- a/app/assets/javascripts/monitoring/components/date_time_picker/date_time_picker.vue +++ b/app/assets/javascripts/monitoring/components/date_time_picker/date_time_picker.vue @@ -55,10 +55,12 @@ export default { }; }, }, - mounted() { - this.verifyTimeRange(); + watch: { + selectedTimeWindow() { + this.verifyTimeRange(); + }, }, - updated() { + mounted() { this.verifyTimeRange(); }, methods: { diff --git a/app/assets/javascripts/new_branch_form.js b/app/assets/javascripts/new_branch_form.js index 52361e963bc..918c6e408a2 100644 --- a/app/assets/javascripts/new_branch_form.js +++ b/app/assets/javascripts/new_branch_form.js @@ -72,16 +72,14 @@ export default class NewBranchForm { }); return `${restriction.prefix} ${formatted.join(restriction.conjunction)}`; }; - const validator = (function(_this) { - return function(errors, restriction) { - const matched = _this.name.val().match(restriction.pattern); - if (matched) { - return errors.concat(formatter(matched.reduce(unique, []), restriction)); - } else { - return errors; - } - }; - })(this); + const validator = (errors, restriction) => { + const matched = this.name.val().match(restriction.pattern); + if (matched) { + return errors.concat(formatter(matched.reduce(unique, []), restriction)); + } else { + return errors; + } + }; const errors = this.restrictions.reduce(validator, []); if (errors.length > 0) { const errorMessage = $('<span/>').text(errors.join(', ')); diff --git a/app/controllers/concerns/renders_commits.rb b/app/controllers/concerns/renders_commits.rb index ed9b898a2a3..826fae834fa 100644 --- a/app/controllers/concerns/renders_commits.rb +++ b/app/controllers/concerns/renders_commits.rb @@ -1,11 +1,11 @@ # frozen_string_literal: true module RendersCommits - def limited_commits(commits) - if commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE + def limited_commits(commits, commits_count) + if commits_count > MergeRequestDiff::COMMITS_SAFE_SIZE [ commits.first(MergeRequestDiff::COMMITS_SAFE_SIZE), - commits.size - MergeRequestDiff::COMMITS_SAFE_SIZE + commits_count - MergeRequestDiff::COMMITS_SAFE_SIZE ] else [commits, 0] @@ -14,9 +14,10 @@ module RendersCommits # This is used as a helper method in a controller. # rubocop: disable Gitlab/ModuleWithInstanceVariables - def set_commits_for_rendering(commits) - @total_commit_count = commits.size - limited, @hidden_commit_count = limited_commits(commits) + def set_commits_for_rendering(commits, commits_count: nil) + @total_commit_count = commits_count || commits.size + limited, @hidden_commit_count = limited_commits(commits, @total_commit_count) + commits.each(&:lazy_author) # preload authors prepare_commits_for_rendering(limited) end # rubocop: enable Gitlab/ModuleWithInstanceVariables diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index 808265634da..78dc196b08e 100644 --- a/app/controllers/projects/merge_requests/creations_controller.rb +++ b/app/controllers/projects/merge_requests/creations_controller.rb @@ -109,7 +109,13 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap @target_project = @merge_request.target_project @source_project = @merge_request.source_project - @commits = set_commits_for_rendering(@merge_request.commits) + + @commits = + set_commits_for_rendering( + @merge_request.recent_commits.with_latest_pipeline(@merge_request.source_branch), + commits_count: @merge_request.commits_count + ) + @commit = @merge_request.diff_head_commit # FIXME: We have to assign a presenter to another instance variable diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 9f6f6621bf4..59987da21ec 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -90,7 +90,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo # Get commits from repository # or from cache if already merged @commits = - set_commits_for_rendering(@merge_request.commits.with_latest_pipeline) + set_commits_for_rendering( + @merge_request.recent_commits.with_latest_pipeline(@merge_request.source_branch), + commits_count: @merge_request.commits_count + ) render json: { html: view_to_html_string('projects/merge_requests/_commits') } end @@ -252,7 +255,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo end def merge_params_attributes - [:should_remove_source_branch, :commit_message, :squash_commit_message, :squash, :auto_merge_strategy] + MergeRequest::KNOWN_MERGE_PARAMS end def auto_merge_requested? @@ -292,7 +295,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo return :sha_mismatch if params[:sha] != @merge_request.diff_head_sha - @merge_request.update(merge_error: nil, squash: merge_params.fetch(:squash, false)) + @merge_request.update(merge_error: nil, squash: params.fetch(:squash, false)) if auto_merge_requested? if merge_request.auto_merge_enabled? diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index cc71ea8e916..e56009be33d 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -117,12 +117,6 @@ class TodosFinder params[:group_id].present? end - def project - strong_memoize(:project) do - Project.find_without_deleted(params[:project_id]) if project? - end - end - def group strong_memoize(:group) do Group.find(params[:group_id]) @@ -181,7 +175,7 @@ class TodosFinder def by_project(items) if project? - items.for_project(project) + items.for_undeleted_projects.for_project(params[:project_id]) else items end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index df516009397..7e1898e7142 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -374,11 +374,12 @@ class MergeRequest < ApplicationRecord "#{project.to_reference(from, full: full)}#{reference}" end - def commits - return merge_request_diff.commits if persisted? + def commits(limit: nil) + return merge_request_diff.commits(limit: limit) if persisted? commits_arr = if compare_commits - compare_commits.reverse + reversed_commits = compare_commits.reverse + limit ? reversed_commits.take(limit) : reversed_commits else [] end @@ -386,6 +387,10 @@ class MergeRequest < ApplicationRecord CommitCollection.new(source_project, commits_arr, source_branch) end + def recent_commits + commits(limit: MergeRequestDiff::COMMITS_SAFE_SIZE) + end + def commits_count if persisted? merge_request_diff.commits_count diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 5fe97a13a42..70ce4df5678 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -213,8 +213,10 @@ class MergeRequestDiff < ApplicationRecord end end - def commits - @commits ||= load_commits + def commits(limit: nil) + strong_memoize(:"commits_#{limit || 'all'}") do + load_commits(limit: limit) + end end def last_commit_sha @@ -529,8 +531,9 @@ class MergeRequestDiff < ApplicationRecord end end - def load_commits - commits = merge_request_diff_commits.map { |commit| Commit.from_hash(commit.to_hash, project) } + def load_commits(limit: nil) + commits = merge_request_diff_commits.limit(limit) + .map { |commit| Commit.from_hash(commit.to_hash, project) } CommitCollection .new(merge_request.source_project, commits, merge_request.source_branch) diff --git a/app/models/project.rb b/app/models/project.rb index 1df2a981658..f4aa336fbcd 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -464,13 +464,6 @@ class Project < ApplicationRecord # Used by Projects::CleanupService to hold a map of rewritten object IDs mount_uploader :bfg_object_map, AttachmentUploader - # Returns a project, if it is not about to be removed. - # - # id - The ID of the project to retrieve. - def self.find_without_deleted(id) - without_deleted.find_by_id(id) - end - def self.eager_load_namespace_and_owner includes(namespace: :owner) end diff --git a/app/models/todo.rb b/app/models/todo.rb index 4e48fb3b782..f217c942e8e 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -55,7 +55,8 @@ class Todo < ApplicationRecord scope :done, -> { with_state(:done) } scope :for_action, -> (action) { where(action: action) } scope :for_author, -> (author) { where(author: author) } - scope :for_project, -> (project) { where(project: project) } + scope :for_project, -> (projects) { where(project: projects) } + scope :for_undeleted_projects, -> { joins(:project).merge(Project.without_deleted) } scope :for_group, -> (group) { where(group: group) } scope :for_type, -> (type) { where(target_type: type) } scope :for_target, -> (id) { where(target_id: id) } diff --git a/app/services/merge_requests/merge_base_service.rb b/app/services/merge_requests/merge_base_service.rb index 3f7f8bcdcbf..27b5e31faab 100644 --- a/app/services/merge_requests/merge_base_service.rb +++ b/app/services/merge_requests/merge_base_service.rb @@ -19,10 +19,12 @@ module MergeRequests end def source - if merge_request.squash - squash_sha! - else - merge_request.diff_head_sha + strong_memoize(:source) do + if merge_request.squash + squash_sha! + else + merge_request.diff_head_sha + end end end @@ -58,16 +60,14 @@ module MergeRequests end def squash_sha! - strong_memoize(:squash_sha) do - params[:merge_request] = merge_request - squash_result = ::MergeRequests::SquashService.new(project, current_user, params).execute - - case squash_result[:status] - when :success - squash_result[:squash_sha] - when :error - raise ::MergeRequests::MergeService::MergeError, squash_result[:message] - end + params[:merge_request] = merge_request + squash_result = ::MergeRequests::SquashService.new(project, current_user, params).execute + + case squash_result[:status] + when :success + squash_result[:squash_sha] + when :error + raise ::MergeRequests::MergeService::MergeError, squash_result[:message] end end end diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 6309052244d..a45b4f1142e 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -37,6 +37,7 @@ module MergeRequests def validate! authorization_check! error_check! + updated_check! end def authorization_check! @@ -60,6 +61,15 @@ module MergeRequests raise_error(error) if error end + def updated_check! + return unless Feature.enabled?(:validate_merge_sha, merge_request.target_project, default_enabled: false) + + unless source_matches? + raise_error('Branch has been updated since the merge was requested. '\ + 'Please review the changes.') + end + end + def commit log_info("Git merge started on JID #{merge_jid}") commit_id = try_merge @@ -125,5 +135,11 @@ module MergeRequests def merge_request_info merge_request.to_reference(full: true) end + + def source_matches? + # params-keys are symbols coming from the controller, but when they get + # loaded from the database they're strings + params.with_indifferent_access[:sha] == merge_request.diff_head_sha + end end end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 7c9abb12b6e..8a6a7119508 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -88,9 +88,9 @@ module MergeRequests merge_request.update(merge_error: nil) if merge_request.head_pipeline && merge_request.head_pipeline.active? - AutoMergeService.new(project, current_user).execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS) + AutoMergeService.new(project, current_user, { sha: last_diff_sha }).execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS) else - merge_request.merge_async(current_user.id, {}) + merge_request.merge_async(current_user.id, { sha: last_diff_sha }) end end diff --git a/changelogs/unreleased/30182-checkbox-with-no-text-causing-following-checkboxes-not-to-be-saved.yml b/changelogs/unreleased/30182-checkbox-with-no-text-causing-following-checkboxes-not-to-be-saved.yml new file mode 100644 index 00000000000..5ea21bbd983 --- /dev/null +++ b/changelogs/unreleased/30182-checkbox-with-no-text-causing-following-checkboxes-not-to-be-saved.yml @@ -0,0 +1,5 @@ +--- +title: Fix checking task item when previous tasks contain only spaces +merge_request: 19724 +author: +type: fixed diff --git a/changelogs/unreleased/32534-gitlab-rake-gitlab-cleanup-orphan_job_artifact_files-dry_run-false-is-not-removing-artifacts.yml b/changelogs/unreleased/32534-gitlab-rake-gitlab-cleanup-orphan_job_artifact_files-dry_run-false-is-not-removing-artifacts.yml new file mode 100644 index 00000000000..aa0e5fee47a --- /dev/null +++ b/changelogs/unreleased/32534-gitlab-rake-gitlab-cleanup-orphan_job_artifact_files-dry_run-false-is-not-removing-artifacts.yml @@ -0,0 +1,5 @@ +--- +title: Correctly cleanup orphan job artifacts +merge_request: 17679 +author: Adam Mulvany +type: fixed diff --git a/changelogs/unreleased/34610-Remove-IIFEs-from-new_branch_form-js.yml b/changelogs/unreleased/34610-Remove-IIFEs-from-new_branch_form-js.yml new file mode 100644 index 00000000000..7875772222f --- /dev/null +++ b/changelogs/unreleased/34610-Remove-IIFEs-from-new_branch_form-js.yml @@ -0,0 +1,5 @@ +--- +title: Remove IIFEs from new_branch_form.js +merge_request: 20009 +author: minghuan lei +type: other diff --git a/changelogs/unreleased/36460-metrics-dashboard-fails-to-load-script-doesn-t-stop.yml b/changelogs/unreleased/36460-metrics-dashboard-fails-to-load-script-doesn-t-stop.yml new file mode 100644 index 00000000000..313df8ba4fb --- /dev/null +++ b/changelogs/unreleased/36460-metrics-dashboard-fails-to-load-script-doesn-t-stop.yml @@ -0,0 +1,5 @@ +--- +title: Remove update hook from date filter to prevent js from getting stuck +merge_request: 20215 +author: +type: fixed diff --git a/changelogs/unreleased/chore-slugify-duplication-removal.yml b/changelogs/unreleased/chore-slugify-duplication-removal.yml new file mode 100644 index 00000000000..a076e7f5429 --- /dev/null +++ b/changelogs/unreleased/chore-slugify-duplication-removal.yml @@ -0,0 +1,5 @@ +--- +title: Remove duplication from slugifyWithUnderscore function +merge_request: 20016 +author: Arun Kumar Mohan +type: other diff --git a/db/migrate/20191114173508_add_resolved_attributes_to_vulnerabilities.rb b/db/migrate/20191114173508_add_resolved_attributes_to_vulnerabilities.rb new file mode 100644 index 00000000000..ec45a729ebb --- /dev/null +++ b/db/migrate/20191114173508_add_resolved_attributes_to_vulnerabilities.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class AddResolvedAttributesToVulnerabilities < ActiveRecord::Migration[5.2] + DOWNTIME = false + + def up + add_column :vulnerabilities, :resolved_by_id, :bigint + add_column :vulnerabilities, :resolved_at, :datetime_with_timezone + end + + def down + remove_column :vulnerabilities, :resolved_at + remove_column :vulnerabilities, :resolved_by_id + end +end diff --git a/db/migrate/20191114173602_add_foreign_key_on_resolved_by_id_to_vulnerabilities.rb b/db/migrate/20191114173602_add_foreign_key_on_resolved_by_id_to_vulnerabilities.rb new file mode 100644 index 00000000000..e0a125ca756 --- /dev/null +++ b/db/migrate/20191114173602_add_foreign_key_on_resolved_by_id_to_vulnerabilities.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class AddForeignKeyOnResolvedByIdToVulnerabilities < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :vulnerabilities, :resolved_by_id + add_concurrent_foreign_key :vulnerabilities, :users, column: :resolved_by_id, on_delete: :nullify + end + + def down + remove_foreign_key :vulnerabilities, column: :resolved_by_id + remove_concurrent_index :vulnerabilities, :resolved_by_id + end +end diff --git a/db/post_migrate/20191114173624_set_resolved_state_on_vulnerabilities.rb b/db/post_migrate/20191114173624_set_resolved_state_on_vulnerabilities.rb new file mode 100644 index 00000000000..b28aecdc0a3 --- /dev/null +++ b/db/post_migrate/20191114173624_set_resolved_state_on_vulnerabilities.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +class SetResolvedStateOnVulnerabilities < ActiveRecord::Migration[5.2] + DOWNTIME = false + + def up + execute <<~SQL + -- selecting IDs for all non-orphan Findings that either have no feedback or it's a non-dismissal feedback + WITH resolved_vulnerability_ids AS ( + SELECT DISTINCT vulnerability_id AS id + FROM vulnerability_occurrences + LEFT JOIN vulnerability_feedback ON vulnerability_feedback.project_fingerprint = ENCODE(vulnerability_occurrences.project_fingerprint::bytea, 'HEX') + WHERE vulnerability_id IS NOT NULL + AND (vulnerability_feedback.id IS NULL OR vulnerability_feedback.feedback_type <> 0) + ) + UPDATE vulnerabilities + SET state = 3, resolved_by_id = closed_by_id, resolved_at = NOW() + FROM resolved_vulnerability_ids + WHERE vulnerabilities.id IN (resolved_vulnerability_ids.id) + AND state = 2 -- only 'closed' Vulnerabilities become 'resolved' + SQL + end + + def down + execute <<~SQL + UPDATE vulnerabilities + SET state = 2, resolved_by_id = NULL, resolved_at = NULL -- state = 'closed' + WHERE state = 3 -- 'resolved' + SQL + end +end diff --git a/db/schema.rb b/db/schema.rb index 72d1d957d6b..ebf1eb41499 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2019_11_12_232338) do +ActiveRecord::Schema.define(version: 2019_11_14_173624) do # These are extensions that must be enabled in order to support this database enable_extension "pg_trgm" @@ -3952,6 +3952,8 @@ ActiveRecord::Schema.define(version: 2019_11_12_232338) do t.boolean "severity_overridden", default: false t.integer "confidence", limit: 2, null: false t.boolean "confidence_overridden", default: false + t.bigint "resolved_by_id" + t.datetime_with_timezone "resolved_at" t.integer "report_type", limit: 2, null: false t.integer "cached_markdown_version" t.index ["author_id"], name: "index_vulnerabilities_on_author_id" @@ -3961,6 +3963,7 @@ ActiveRecord::Schema.define(version: 2019_11_12_232338) do t.index ["last_edited_by_id"], name: "index_vulnerabilities_on_last_edited_by_id" t.index ["milestone_id"], name: "index_vulnerabilities_on_milestone_id" t.index ["project_id"], name: "index_vulnerabilities_on_project_id" + t.index ["resolved_by_id"], name: "index_vulnerabilities_on_resolved_by_id" t.index ["start_date_sourcing_milestone_id"], name: "index_vulnerabilities_on_start_date_sourcing_milestone_id" t.index ["updated_by_id"], name: "index_vulnerabilities_on_updated_by_id" end @@ -4518,6 +4521,7 @@ ActiveRecord::Schema.define(version: 2019_11_12_232338) do add_foreign_key "vulnerabilities", "users", column: "author_id", name: "fk_b1de915a15", on_delete: :nullify add_foreign_key "vulnerabilities", "users", column: "closed_by_id", name: "fk_cf5c60acbf", on_delete: :nullify add_foreign_key "vulnerabilities", "users", column: "last_edited_by_id", name: "fk_1302949740", on_delete: :nullify + add_foreign_key "vulnerabilities", "users", column: "resolved_by_id", name: "fk_76bc5f5455", on_delete: :nullify add_foreign_key "vulnerabilities", "users", column: "updated_by_id", name: "fk_7ac31eacb9", on_delete: :nullify add_foreign_key "vulnerability_feedback", "ci_pipelines", column: "pipeline_id", on_delete: :nullify add_foreign_key "vulnerability_feedback", "issues", on_delete: :nullify diff --git a/doc/development/documentation/styleguide.md b/doc/development/documentation/styleguide.md index 8d68079963f..e6d666473c3 100644 --- a/doc/development/documentation/styleguide.md +++ b/doc/development/documentation/styleguide.md @@ -1179,12 +1179,12 @@ Rendered example: - Prefer to use examples using the personal access token and don't pass data of username and password. -| Methods | Description | -|:-------------------------------------------|:------------------------------------------------------| -| `-H "PRIVATE-TOKEN: <your_access_token>"` | Use this method as is, whenever authentication needed | -| `-X POST` | Use this method when creating new objects | -| `-X PUT` | Use this method when updating existing objects | -| `-X DELETE` | Use this method when removing existing objects | +| Methods | Description | +|:------------------------------------------- |:------------------------------------------------------| +| `--header "PRIVATE-TOKEN: <your_access_token>"` | Use this method as is, whenever authentication needed | +| `--request POST` | Use this method when creating new objects | +| `--request PUT` | Use this method when updating existing objects | +| `--request DELETE` | Use this method when removing existing objects | ### cURL Examples @@ -1206,9 +1206,9 @@ Create a new project under the authenticated user's namespace: curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/projects?name=foo" ``` -#### Post data using cURL's --data +#### Post data using cURL's `--data` -Instead of using `-X POST` and appending the parameters to the URI, you can use +Instead of using `--request POST` and appending the parameters to the URI, you can use cURL's `--data` option. The example below will create a new project `foo` under the authenticated user's namespace. diff --git a/doc/development/geo.md b/doc/development/geo.md index 2fb4cc710ff..5010e44e826 100644 --- a/doc/development/geo.md +++ b/doc/development/geo.md @@ -491,6 +491,24 @@ When some write actions are not allowed because the node is a The database itself will already be read-only in a replicated setup, so we don't need to take any extra step for that. +## Steps needed to replicate a new data type + +As GitLab evolves, we constantly need to add new resources to the Geo replication system. +The implementation depends on resource specifics, but there are several things +that need to be taken care of: + +- Event generation on the primary site. Whenever a new resource is changed/updated, we need to + create a task for the Log Cursor. +- Event handling. The Log Cursor needs to have a handler for every event type generated by the primary site. +- Dispatch worker (cron job). Make sure the backfill condition works well. +- Sync worker. +- Registry with all possible states. +- Verification. +- Cleaner. When sync settings are changed for the secondary site, some resources need to be cleaned up. +- Geo Node Status. We need to provide API endpoints as well as some presentation in the GitLab Admin Area. +- Health Check. If we can perform some pre-cheŃks and make node unhealthy if something is wrong, we should do that. + The `rake gitlab:geo:check` command has to be updated too. + ## History of communication channel The communication channel has changed since first iteration, you can diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 47ee81c7080..6e10414def4 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -407,7 +407,8 @@ module API merge_params = HashWithIndifferentAccess.new( commit_message: params[:merge_commit_message], squash_commit_message: params[:squash_commit_message], - should_remove_source_branch: params[:should_remove_source_branch] + should_remove_source_branch: params[:should_remove_source_branch], + sha: params[:sha] || merge_request.diff_head_sha ) if merge_when_pipeline_succeeds && merge_request.head_pipeline && merge_request.head_pipeline.active? diff --git a/lib/gitlab/cleanup/orphan_job_artifact_files.rb b/lib/gitlab/cleanup/orphan_job_artifact_files.rb index 1b01ca25559..020de45e5bf 100644 --- a/lib/gitlab/cleanup/orphan_job_artifact_files.rb +++ b/lib/gitlab/cleanup/orphan_job_artifact_files.rb @@ -8,7 +8,8 @@ module Gitlab ABSOLUTE_ARTIFACT_DIR = ::JobArtifactUploader.root.freeze LOST_AND_FOUND = File.join(ABSOLUTE_ARTIFACT_DIR, '-', 'lost+found').freeze BATCH_SIZE = 500 - DEFAULT_NICENESS = 'Best-effort' + DEFAULT_NICENESS = 'best-effort' + VALID_NICENESS_LEVELS = %w{none realtime best-effort idle}.freeze attr_accessor :batch, :total_found, :total_cleaned attr_reader :limit, :dry_run, :niceness, :logger @@ -16,7 +17,7 @@ module Gitlab def initialize(limit: nil, dry_run: true, niceness: nil, logger: nil) @limit = limit @dry_run = dry_run - @niceness = niceness || DEFAULT_NICENESS + @niceness = (niceness || DEFAULT_NICENESS).downcase @logger = logger || Rails.logger # rubocop:disable Gitlab/RailsLogger @total_found = @total_cleaned = 0 @@ -35,7 +36,7 @@ module Gitlab clean_batch! - log_info("Processed #{total_found} job artifacts to find and clean #{total_cleaned} orphans.") + log_info("Processed #{total_found} job artifact(s) to find and cleaned #{total_cleaned} orphan(s).") end private @@ -75,7 +76,7 @@ module Gitlab def find_artifacts Open3.popen3(*find_command) do |stdin, stdout, stderr, status_thread| stdout.each_line do |line| - yield line + yield line.chomp end log_error(stderr.read.color(:red)) unless status_thread.value.success? @@ -99,7 +100,7 @@ module Gitlab cmd += %w[-type d] if ionice - raise ArgumentError, 'Invalid niceness' unless niceness.match?(/^\w[\w\-]*$/) + raise ArgumentError, 'Invalid niceness' unless VALID_NICENESS_LEVELS.include?(niceness) cmd.unshift(*%W[#{ionice} --class #{niceness}]) end diff --git a/package.json b/package.json index f247b972bd2..2a439be9514 100644 --- a/package.json +++ b/package.json @@ -75,7 +75,7 @@ "d3-time-format": "^2.1.1", "d3-transition": "^1.1.1", "dateformat": "^3.0.3", - "deckar01-task_list": "^2.2.0", + "deckar01-task_list": "^2.2.1", "diff": "^3.4.0", "document-register-element": "1.13.1", "dropzone": "^4.2.0", diff --git a/spec/controllers/concerns/renders_commits_spec.rb b/spec/controllers/concerns/renders_commits_spec.rb new file mode 100644 index 00000000000..79350847383 --- /dev/null +++ b/spec/controllers/concerns/renders_commits_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe RendersCommits do + let_it_be(:project) { create(:project, :public, :repository) } + let_it_be(:merge_request) { create(:merge_request, source_project: project) } + let_it_be(:user) { create(:user) } + + controller(ApplicationController) do + # `described_class` is not available in this context + include RendersCommits # rubocop:disable RSpec/DescribedClass + + def index + @merge_request = MergeRequest.find(params[:id]) + @commits = set_commits_for_rendering( + @merge_request.recent_commits.with_latest_pipeline(@merge_request.source_branch), + commits_count: @merge_request.commits_count + ) + + render json: { html: view_to_html_string('projects/merge_requests/_commits') } + end + end + + before do + sign_in(user) + end + + def go + get :index, params: { id: merge_request.id } + end + + it 'sets instance variables for counts' do + stub_const("MergeRequestDiff::COMMITS_SAFE_SIZE", 10) + + go + + expect(assigns[:total_commit_count]).to eq(29) + expect(assigns[:hidden_commit_count]).to eq(19) + expect(assigns[:commits].size).to eq(10) + end + + context 'rendering commits' do + render_views + + it 'avoids N + 1' do + stub_const("MergeRequestDiff::COMMITS_SAFE_SIZE", 5) + + control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do + go + end.count + + stub_const("MergeRequestDiff::COMMITS_SAFE_SIZE", 15) + + expect do + go + end.not_to exceed_all_query_limit(control_count) + end + end +end diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index e49c0f60eeb..9f7fde2f0da 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -405,7 +405,7 @@ describe Projects::MergeRequestsController do end it 'starts the merge immediately with permitted params' do - expect(MergeWorker).to receive(:perform_async).with(merge_request.id, anything, { 'squash' => false }) + expect(MergeWorker).to receive(:perform_async).with(merge_request.id, anything, { 'sha' => merge_request.diff_head_sha }) merge_with_sha end @@ -432,9 +432,14 @@ describe Projects::MergeRequestsController do let(:message) { 'My custom squash commit message' } it 'passes the same message to SquashService', :sidekiq_might_not_need_inline do - params = { squash: '1', squash_commit_message: message } - - expect_next_instance_of(MergeRequests::SquashService, project, user, params.merge(merge_request: merge_request)) do |squash_service| + params = { squash: '1', + squash_commit_message: message, + sha: merge_request.diff_head_sha } + expected_squash_params = { squash_commit_message: message, + sha: merge_request.diff_head_sha, + merge_request: merge_request } + + expect_next_instance_of(MergeRequests::SquashService, project, user, expected_squash_params) do |squash_service| expect(squash_service).to receive(:execute).and_return({ status: :success, squash_sha: SecureRandom.hex(20) diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index 13612214e72..42248dc1165 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -100,6 +100,7 @@ FactoryBot.define do auto_merge_enabled { true } auto_merge_strategy { AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS } merge_user { author } + merge_params { { sha: diff_head_sha } } end trait :remove_source_branch do diff --git a/spec/features/merge_request/user_sees_cherry_pick_modal_spec.rb b/spec/features/merge_request/user_sees_cherry_pick_modal_spec.rb index 1d62f7f0702..d7675cd06a8 100644 --- a/spec/features/merge_request/user_sees_cherry_pick_modal_spec.rb +++ b/spec/features/merge_request/user_sees_cherry_pick_modal_spec.rb @@ -15,7 +15,7 @@ describe 'Merge request > User cherry-picks', :js do context 'Viewing a merged merge request' do before do - service = MergeRequests::MergeService.new(project, user) + service = MergeRequests::MergeService.new(project, user, sha: merge_request.diff_head_sha) perform_enqueued_jobs do service.execute(merge_request) diff --git a/spec/finders/todos_finder_spec.rb b/spec/finders/todos_finder_spec.rb index 75aa10c66a2..a837e7af251 100644 --- a/spec/finders/todos_finder_spec.rb +++ b/spec/finders/todos_finder_spec.rb @@ -163,6 +163,28 @@ describe TodosFinder do expect(todos).to match_array([todo1, todo2]) end end + + context 'by project' do + let_it_be(:project1) { create(:project) } + let_it_be(:project2) { create(:project) } + let_it_be(:project3) { create(:project) } + + let!(:todo1) { create(:todo, user: user, project: project1, state: :pending) } + let!(:todo2) { create(:todo, user: user, project: project2, state: :pending) } + let!(:todo3) { create(:todo, user: user, project: project3, state: :pending) } + + it 'returns the expected todos for one project' do + todos = finder.new(user, { project_id: project2.id }).execute + + expect(todos).to match_array([todo2]) + end + + it 'returns the expected todos for many projects' do + todos = finder.new(user, { project_id: [project2.id, project1.id] }).execute + + expect(todos).to match_array([todo2, todo1]) + end + end end context 'external authorization' do diff --git a/spec/frontend/fixtures/merge_requests.rb b/spec/frontend/fixtures/merge_requests.rb index 8fbdb534b3d..f20c0aa3540 100644 --- a/spec/frontend/fixtures/merge_requests.rb +++ b/spec/frontend/fixtures/merge_requests.rb @@ -8,7 +8,23 @@ describe Projects::MergeRequestsController, '(JavaScript fixtures)', type: :cont let(:admin) { create(:admin) } let(:namespace) { create(:namespace, name: 'frontend-fixtures' )} let(:project) { create(:project, :repository, namespace: namespace, path: 'merge-requests-project') } - let(:merge_request) { create(:merge_request, :with_diffs, source_project: project, target_project: project, description: '- [ ] Task List Item') } + + # rubocop: disable Layout/TrailingWhitespace + let(:merge_request) do + create( + :merge_request, + :with_diffs, + source_project: project, + target_project: project, + description: <<~MARKDOWN.strip_heredoc + - [ ] Task List Item + - [ ] + - [ ] Task List Item 2 + MARKDOWN + ) + end + # rubocop: enable Layout/TrailingWhitespace + let(:merged_merge_request) { create(:merge_request, :merged, source_project: project, target_project: project) } let(:pipeline) do create( diff --git a/spec/frontend/monitoring/components/date_time_picker/date_time_picker_spec.js b/spec/frontend/monitoring/components/date_time_picker/date_time_picker_spec.js index be544435671..ca05461c8cf 100644 --- a/spec/frontend/monitoring/components/date_time_picker/date_time_picker_spec.js +++ b/spec/frontend/monitoring/components/date_time_picker/date_time_picker_spec.js @@ -51,6 +51,16 @@ describe('DateTimePicker', () => { }); }); + it('renders dropdown without a selectedTimeWindow set', done => { + createComponent({ + selectedTimeWindow: {}, + }); + dateTimePicker.vm.$nextTick(() => { + expect(dateTimePicker.findAll('input').length).toBe(2); + done(); + }); + }); + it('renders inputs with h/m/s truncated if its all 0s', done => { createComponent({ selectedTimeWindow: { diff --git a/spec/javascripts/merge_request_spec.js b/spec/javascripts/merge_request_spec.js index 998637ef595..54071ccc5c2 100644 --- a/spec/javascripts/merge_request_spec.js +++ b/spec/javascripts/merge_request_spec.js @@ -1,5 +1,3 @@ -/* eslint-disable no-return-assign */ - import $ from 'jquery'; import MockAdapter from 'axios-mock-adapter'; import axios from '~/lib/utils/axios_utils'; @@ -22,7 +20,8 @@ describe('MergeRequest', function() { .onPatch(`${gl.TEST_HOST}/frontend-fixtures/merge-requests-project/merge_requests/1.json`) .reply(200, {}); - return (this.merge = new MergeRequest()); + this.merge = new MergeRequest(); + return this.merge; }); afterEach(() => { @@ -34,10 +33,30 @@ describe('MergeRequest', function() { const changeEvent = document.createEvent('HTMLEvents'); changeEvent.initEvent('change', true, true); $('input[type=checkbox]') + .first() + .attr('checked', true)[0] + .dispatchEvent(changeEvent); + setTimeout(() => { + expect($('.js-task-list-field').val()).toBe( + '- [x] Task List Item\n- [ ] \n- [ ] Task List Item 2\n', + ); + done(); + }); + }); + + it('ensure that task with only spaces does not get checked incorrectly', done => { + // fixed in 'deckar01-task_list', '2.2.1' gem + spyOn($, 'ajax').and.stub(); + const changeEvent = document.createEvent('HTMLEvents'); + changeEvent.initEvent('change', true, true); + $('input[type=checkbox]') + .last() .attr('checked', true)[0] .dispatchEvent(changeEvent); setTimeout(() => { - expect($('.js-task-list-field').val()).toBe('- [x] Task List Item'); + expect($('.js-task-list-field').val()).toBe( + '- [ ] Task List Item\n- [ ] \n- [x] Task List Item 2\n', + ); done(); }); }); @@ -59,7 +78,7 @@ describe('MergeRequest', function() { `${gl.TEST_HOST}/frontend-fixtures/merge-requests-project/merge_requests/1.json`, { merge_request: { - description: '- [ ] Task List Item', + description: '- [ ] Task List Item\n- [ ] \n- [ ] Task List Item 2\n', lock_version: 0, update_task: { line_number: lineNumber, line_source: lineSource, index, checked }, }, diff --git a/spec/lib/gitlab/cleanup/orphan_job_artifact_files_spec.rb b/spec/lib/gitlab/cleanup/orphan_job_artifact_files_spec.rb index 974cc2c4660..fc9792e16d7 100644 --- a/spec/lib/gitlab/cleanup/orphan_job_artifact_files_spec.rb +++ b/spec/lib/gitlab/cleanup/orphan_job_artifact_files_spec.rb @@ -21,11 +21,10 @@ describe Gitlab::Cleanup::OrphanJobArtifactFiles do end it 'errors when invalid niceness is given' do + allow(Gitlab::Utils).to receive(:which).with('ionice').and_return('/fake/ionice') cleanup = described_class.new(logger: null_logger, niceness: 'FooBar') - expect(null_logger).to receive(:error).with(/FooBar/) - - cleanup.run! + expect { cleanup.run! }.to raise_error('Invalid niceness') end it 'finds artifacts on disk' do @@ -63,6 +62,8 @@ describe Gitlab::Cleanup::OrphanJobArtifactFiles do def mock_artifacts_found(cleanup, *files) mock = allow(cleanup).to receive(:find_artifacts) - files.each { |file| mock.and_yield(file) } + # Because we shell out to run `find -L ...`, each file actually + # contains a trailing newline + files.each { |file| mock.and_yield("#{file}\n") } end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index b19f7a80d63..b5aa05fd8b4 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1934,7 +1934,7 @@ describe MergeRequest do context 'when the MR has been merged' do before do MergeRequests::MergeService - .new(subject.target_project, subject.author) + .new(subject.target_project, subject.author, { sha: subject.diff_head_sha }) .execute(subject) end @@ -3484,4 +3484,67 @@ describe MergeRequest do end it_behaves_like 'versioned description' + + describe '#commits' do + context 'persisted merge request' do + context 'with a limit' do + it 'returns a limited number of commits' do + expect(subject.commits(limit: 2).map(&:sha)).to eq(%w[ + b83d6e391c22777fca1ed3012fce84f633d7fed0 + 498214de67004b1da3d820901307bed2a68a8ef6 + ]) + expect(subject.commits(limit: 3).map(&:sha)).to eq(%w[ + b83d6e391c22777fca1ed3012fce84f633d7fed0 + 498214de67004b1da3d820901307bed2a68a8ef6 + 1b12f15a11fc6e62177bef08f47bc7b5ce50b141 + ]) + end + end + + context 'without a limit' do + it 'returns all commits of the merge request diff' do + expect(subject.commits.size).to eq(29) + end + end + end + + context 'new merge request' do + subject { build(:merge_request) } + + context 'compare commits' do + let(:first_commit) { double } + let(:second_commit) { double } + + before do + subject.compare_commits = [ + first_commit, second_commit + ] + end + + context 'without a limit' do + it 'returns all the compare commits' do + expect(subject.commits.to_a).to eq([second_commit, first_commit]) + end + end + + context 'with a limit' do + it 'returns a limited number of commits' do + expect(subject.commits(limit: 1).to_a).to eq([second_commit]) + end + end + end + end + end + + describe '#recent_commits' do + before do + stub_const("#{MergeRequestDiff}::COMMITS_SAFE_SIZE", 2) + end + + it 'returns the safe number of commits' do + expect(subject.recent_commits.map(&:sha)).to eq(%w[ + b83d6e391c22777fca1ed3012fce84f633d7fed0 498214de67004b1da3d820901307bed2a68a8ef6 + ]) + end + end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 0a64c70dccb..815ab7aa166 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -4899,20 +4899,6 @@ describe Project do end end - describe '.find_without_deleted' do - it 'returns nil if the project is about to be removed' do - project = create(:project, pending_delete: true) - - expect(described_class.find_without_deleted(project.id)).to be_nil - end - - it 'returns a project when it is not about to be removed' do - project = create(:project) - - expect(described_class.find_without_deleted(project.id)).to eq(project) - end - end - describe '.for_group' do it 'returns the projects for a given group' do group = create(:group) diff --git a/spec/models/todo_spec.rb b/spec/models/todo_spec.rb index e79a2bc325b..ea09c6caed3 100644 --- a/spec/models/todo_spec.rb +++ b/spec/models/todo_spec.rb @@ -221,6 +221,40 @@ describe Todo do expect(described_class.for_project(project1)).to eq([todo]) end + + it 'returns the todos for many projects' do + project1 = create(:project) + project2 = create(:project) + project3 = create(:project) + + todo1 = create(:todo, project: project1) + todo2 = create(:todo, project: project2) + create(:todo, project: project3) + + expect(described_class.for_project([project2, project1])).to contain_exactly(todo2, todo1) + end + end + + describe '.for_undeleted_projects' do + let(:project1) { create(:project) } + let(:project2) { create(:project) } + let(:project3) { create(:project) } + + let!(:todo1) { create(:todo, project: project1) } + let!(:todo2) { create(:todo, project: project2) } + let!(:todo3) { create(:todo, project: project3) } + + it 'returns the todos for a given project' do + expect(described_class.for_undeleted_projects).to contain_exactly(todo1, todo2, todo3) + end + + context 'when todo belongs to deleted project' do + let(:project2) { create(:project, pending_delete: true) } + + it 'excludes todos of deleted projects' do + expect(described_class.for_undeleted_projects).to contain_exactly(todo1, todo3) + end + end end describe '.for_group' do diff --git a/spec/services/merge_requests/ff_merge_service_spec.rb b/spec/services/merge_requests/ff_merge_service_spec.rb index 3b1096c51cb..c724a1a47b4 100644 --- a/spec/services/merge_requests/ff_merge_service_spec.rb +++ b/spec/services/merge_requests/ff_merge_service_spec.rb @@ -13,6 +13,7 @@ describe MergeRequests::FfMergeService do author: create(:user)) end let(:project) { merge_request.project } + let(:valid_merge_params) { { sha: merge_request.diff_head_sha } } before do project.add_maintainer(user) @@ -21,7 +22,7 @@ describe MergeRequests::FfMergeService do describe '#execute' do context 'valid params' do - let(:service) { described_class.new(project, user, {}) } + let(:service) { described_class.new(project, user, valid_merge_params) } before do allow(service).to receive(:execute_hooks) @@ -52,8 +53,8 @@ describe MergeRequests::FfMergeService do end end - context "error handling" do - let(:service) { described_class.new(project, user, commit_message: 'Awesome message') } + context 'error handling' do + let(:service) { described_class.new(project, user, valid_merge_params.merge(commit_message: 'Awesome message')) } before do allow(Rails.logger).to receive(:error) diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 22578436c18..c938dd1cb0b 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -14,9 +14,12 @@ describe MergeRequests::MergeService do end describe '#execute' do - context 'valid params' do - let(:service) { described_class.new(project, user, commit_message: 'Awesome message') } + let(:service) { described_class.new(project, user, merge_params) } + let(:merge_params) do + { commit_message: 'Awesome message', sha: merge_request.diff_head_sha } + end + context 'valid params' do before do allow(service).to receive(:execute_hooks) @@ -38,11 +41,80 @@ describe MergeRequests::MergeService do note = merge_request.notes.last expect(note.note).to include 'merged' end + + context 'when squashing' do + let(:merge_params) do + { commit_message: 'Merge commit message', + squash_commit_message: 'Squash commit message', + sha: merge_request.diff_head_sha } + end + + let(:merge_request) do + # A merge reqeust with 5 commits + create(:merge_request, :simple, + author: user2, + assignees: [user2], + squash: true, + source_branch: 'improve/awesome', + target_branch: 'fix') + end + + it 'merges the merge request with squashed commits' do + expect(merge_request).to be_merged + + merge_commit = merge_request.merge_commit + squash_commit = merge_request.merge_commit.parents.last + + expect(merge_commit.message).to eq('Merge commit message') + expect(squash_commit.message).to eq("Squash commit message\n") + end + end end - context 'closes related issues' do - let(:service) { described_class.new(project, user, commit_message: 'Awesome message') } + context 'when an invalid sha is passed' do + let(:merge_request) do + create(:merge_request, :simple, + author: user2, + assignees: [user2], + squash: true, + source_branch: 'improve/awesome', + target_branch: 'fix') + end + + let(:merge_params) do + { sha: merge_request.commits.second.sha } + end + + it 'does not merge the MR' do + service.execute(merge_request) + + expect(merge_request).not_to be_merged + expect(merge_request.merge_error).to match(/Branch has been updated/) + end + end + + context 'when the `sha` param is missing' do + let(:merge_params) { {} } + + it 'returns the error' do + merge_error = 'Branch has been updated since the merge was requested. '\ + 'Please review the changes.' + + expect { service.execute(merge_request) } + .to change { merge_request.merge_error } + .from(nil).to(merge_error) + end + + it 'merges the MR when the feature is disabled' do + stub_feature_flags(validate_merge_sha: false) + service.execute(merge_request) + + expect(merge_request).to be_merged + end + end + + context 'closes related issues' do before do allow(project).to receive(:default_branch).and_return(merge_request.target_branch) end @@ -83,12 +155,12 @@ describe MergeRequests::MergeService do service.execute(merge_request) end - context "when jira_issue_transition_id is not present" do + context 'when jira_issue_transition_id is not present' do before do allow_any_instance_of(JIRA::Resource::Issue).to receive(:resolution).and_return(nil) end - it "does not close issue" do + it 'does not close issue' do allow(jira_tracker).to receive_messages(jira_issue_transition_id: nil) expect_any_instance_of(JiraService).not_to receive(:transition_issue) @@ -97,7 +169,7 @@ describe MergeRequests::MergeService do end end - context "wrong issue markdown" do + context 'wrong issue markdown' do it 'does not close issues on Jira issue tracker' do jira_issue = ExternalIssue.new('#JIRA-123', project) stub_jira_urls(jira_issue) @@ -115,7 +187,7 @@ describe MergeRequests::MergeService do context 'closes related todos' do let(:merge_request) { create(:merge_request, assignees: [user], author: user) } let(:project) { merge_request.project } - let(:service) { described_class.new(project, user, commit_message: 'Awesome message') } + let!(:todo) do create(:todo, :assigned, project: project, @@ -139,7 +211,7 @@ describe MergeRequests::MergeService do context 'source branch removal' do context 'when the source branch is protected' do let(:service) do - described_class.new(project, user, 'should_remove_source_branch' => true) + described_class.new(project, user, merge_params.merge('should_remove_source_branch' => true)) end before do @@ -154,7 +226,7 @@ describe MergeRequests::MergeService do context 'when the source branch is the default branch' do let(:service) do - described_class.new(project, user, 'should_remove_source_branch' => true) + described_class.new(project, user, merge_params.merge('should_remove_source_branch' => true)) end before do @@ -169,8 +241,6 @@ describe MergeRequests::MergeService do context 'when the source branch can be removed' do context 'when MR author set the source branch to be removed' do - let(:service) { described_class.new(project, user, commit_message: 'Awesome message') } - before do merge_request.update_attribute(:merge_params, { 'force_remove_source_branch' => '1' }) end @@ -183,7 +253,7 @@ describe MergeRequests::MergeService do end context 'when the merger set the source branch not to be removed' do - let(:service) { described_class.new(project, user, commit_message: 'Awesome message', 'should_remove_source_branch' => false) } + let(:service) { described_class.new(project, user, merge_params.merge('should_remove_source_branch' => false)) } it 'does not delete the source branch' do expect(DeleteBranchService).not_to receive(:new) @@ -194,7 +264,7 @@ describe MergeRequests::MergeService do context 'when MR merger set the source branch to be removed' do let(:service) do - described_class.new(project, user, commit_message: 'Awesome message', 'should_remove_source_branch' => true) + described_class.new(project, user, merge_params.merge('should_remove_source_branch' => true)) end it 'removes the source branch using the current user' do @@ -207,9 +277,7 @@ describe MergeRequests::MergeService do end end - context "error handling" do - let(:service) { described_class.new(project, user, commit_message: 'Awesome message') } - + context 'error handling' do before do allow(Rails.logger).to receive(:error) end @@ -230,7 +298,7 @@ describe MergeRequests::MergeService do it 'logs and saves error if there is an exception' do error_message = 'error message' - allow(service).to receive(:repository).and_raise("error message") + allow(service).to receive(:repository).and_raise('error message') allow(service).to receive(:execute_hooks) service.execute(merge_request) @@ -310,7 +378,7 @@ describe MergeRequests::MergeService do expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) end - context "when fast-forward merge is not allowed" do + context 'when fast-forward merge is not allowed' do before do allow_any_instance_of(Repository).to receive(:ancestor?).and_return(nil) end diff --git a/spec/services/merge_requests/merge_to_ref_service_spec.rb b/spec/services/merge_requests/merge_to_ref_service_spec.rb index 758679edc45..cccafddc450 100644 --- a/spec/services/merge_requests/merge_to_ref_service_spec.rb +++ b/spec/services/merge_requests/merge_to_ref_service_spec.rb @@ -76,7 +76,7 @@ describe MergeRequests::MergeToRefService do described_class.new(project, user, **params) end - let(:params) { { commit_message: 'Awesome message', should_remove_source_branch: true } } + let(:params) { { commit_message: 'Awesome message', should_remove_source_branch: true, sha: merge_request.diff_head_sha } } def process_merge_to_ref perform_enqueued_jobs do @@ -103,7 +103,7 @@ describe MergeRequests::MergeToRefService do end let(:merge_service) do - MergeRequests::MergeService.new(project, user, {}) + MergeRequests::MergeService.new(project, user, { sha: merge_request.diff_head_sha }) end context 'when merge commit' do @@ -205,7 +205,7 @@ describe MergeRequests::MergeToRefService do end context 'when target ref is passed as a parameter' do - let(:params) { { commit_message: 'merge train', target_ref: target_ref } } + let(:params) { { commit_message: 'merge train', target_ref: target_ref, sha: merge_request.diff_head_sha } } it_behaves_like 'successfully merges to ref with merge method' do let(:first_parent_ref) { 'refs/heads/master' } @@ -215,7 +215,7 @@ describe MergeRequests::MergeToRefService do describe 'cascading merge refs' do set(:project) { create(:project, :repository) } - let(:params) { { commit_message: 'Cascading merge', first_parent_ref: first_parent_ref, target_ref: target_ref } } + let(:params) { { commit_message: 'Cascading merge', first_parent_ref: first_parent_ref, target_ref: target_ref, sha: merge_request.diff_head_sha } } context 'when first merge happens' do let(:merge_request) do diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 0cfcae1b006..baa0ecf27e3 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -219,7 +219,7 @@ describe MergeRequests::UpdateService, :mailer do head_pipeline_of: merge_request ) - expect(AutoMerge::MergeWhenPipelineSucceedsService).to receive(:new).with(project, user, {}) + expect(AutoMerge::MergeWhenPipelineSucceedsService).to receive(:new).with(project, user, { sha: merge_request.diff_head_sha }) .and_return(service_mock) allow(service_mock).to receive(:available_for?) { true } expect(service_mock).to receive(:execute).with(merge_request) @@ -411,7 +411,7 @@ describe MergeRequests::UpdateService, :mailer do context 'when auto merge is enabled and target branch changed' do before do - AutoMergeService.new(project, user).execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS) + AutoMergeService.new(project, user, { sha: merge_request.diff_head_sha }).execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS) update_merge_request({ target_branch: 'target' }) end diff --git a/spec/support/helpers/cycle_analytics_helpers.rb b/spec/support/helpers/cycle_analytics_helpers.rb index a604359942f..d101b092e7d 100644 --- a/spec/support/helpers/cycle_analytics_helpers.rb +++ b/spec/support/helpers/cycle_analytics_helpers.rb @@ -77,7 +77,7 @@ module CycleAnalyticsHelpers .new(project, user) .closed_by_merge_requests(issue) - merge_requests.each { |merge_request| MergeRequests::MergeService.new(project, user).execute(merge_request) } + merge_requests.each { |merge_request| MergeRequests::MergeService.new(project, user, sha: merge_request.diff_head_sha).execute(merge_request) } end def deploy_master(user, project, environment: 'production') diff --git a/spec/workers/merge_worker_spec.rb b/spec/workers/merge_worker_spec.rb index 138a99abde6..dc98c9836fa 100644 --- a/spec/workers/merge_worker_spec.rb +++ b/spec/workers/merge_worker_spec.rb @@ -20,6 +20,7 @@ describe MergeWorker do described_class.new.perform( merge_request.id, merge_request.author_id, commit_message: 'wow such merge', + sha: merge_request.diff_head_sha, should_remove_source_branch: true) merge_request.reload diff --git a/spec/workers/process_commit_worker_spec.rb b/spec/workers/process_commit_worker_spec.rb index eb1d3c364ac..99800135075 100644 --- a/spec/workers/process_commit_worker_spec.rb +++ b/spec/workers/process_commit_worker_spec.rb @@ -81,9 +81,10 @@ describe ProcessCommitWorker do let(:commit) do project.repository.create_branch('feature-merged', 'feature') + project.repository.after_create_branch MergeRequests::MergeService - .new(project, merge_request.author) + .new(project, merge_request.author, { sha: merge_request.diff_head_sha }) .execute(merge_request) merge_request.reload.merge_commit diff --git a/yarn.lock b/yarn.lock index ba6a14cbba0..61173c11bf3 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3614,10 +3614,10 @@ decamelize@^1.1.0, decamelize@^1.1.1, decamelize@^1.1.2, decamelize@^1.2.0: resolved "https://registry.yarnpkg.com/decamelize/-/decamelize-1.2.0.tgz#f6534d15148269b20352e7bee26f501f9a191290" integrity sha1-9lNNFRSCabIDUue+4m9QH5oZEpA= -deckar01-task_list@^2.2.0: - version "2.2.0" - resolved "https://registry.yarnpkg.com/deckar01-task_list/-/deckar01-task_list-2.2.0.tgz#5cc3ea06f01d3d786b1a667064a462eb5d069bd3" - integrity sha512-NUfu5ARoD9SC2k+fBT5cBer59iKfEdawPrmfqp5+zAahTECb8z9dsuS1Xnx7jzFAmCCLnEs3z/aYucYXzNrKkQ== +deckar01-task_list@^2.2.1: + version "2.2.1" + resolved "https://registry.yarnpkg.com/deckar01-task_list/-/deckar01-task_list-2.2.1.tgz#e1e8a16c4fd6e153e51fd9258fdbee067ebcd86b" + integrity sha512-aNAVYAYwONXezSQy2p5M67wjZE+U7JpPotdegbyy1Wq35V6jDhF3qndJYA1rYnY3aI9ifCep6EMGPav/UQaBZw== decode-uri-component@^0.2.0: version "0.2.0" |