diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index 214e87052da66d5c248328621b04c27d9eaa3118..07a7b5ce9de1f52f018cb2bd799581b66ffd3e8d 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/app/views/shared/issuable/_form.html.haml @@ -33,6 +33,8 @@ = render_if_exists 'shared/issuable/approvals', issuable: issuable, presenter: presenter, form: form += render_if_exists "shared/issuable/form/merge_request_blocks", issuable: issuable, form: form + = render 'shared/issuable/form/branch_chooser', issuable: issuable, form: form = render 'shared/issuable/form/merge_params', issuable: issuable diff --git a/doc/user/project/merge_requests/blocking_merge_requests.md b/doc/user/project/merge_requests/blocking_merge_requests.md new file mode 100644 index 0000000000000000000000000000000000000000..0506a7cb4a5b2bd80c18f2ce51ec2ce27395f431 --- /dev/null +++ b/doc/user/project/merge_requests/blocking_merge_requests.md @@ -0,0 +1,133 @@ +--- +type: reference, concepts +--- + +# Blocking merge requests **(PREMIUM)** + +> Introduced in GitLab Premium 12.2 + +Blocking merge requests allow dependencies between MRs to be expressed. If a +merge request is blocked by another MR, it cannot be merged until that blocking +MR is itself merged. + +NOTE: **Note:** +Blocking merge requests are a **PREMIUM** feature, but this restriction is only +enforced for the blocked merge request. A merge request in a **CORE** or +**STARTER** project can block a **PREMIUM** merge request, but not vice-versa. + +## Use cases + +* Ensure changes to a library are merged before changes to a project that + imports the library +* Prevent a documentation-only merge request from being merged before the MR + implementing the feature to be documented +* Require an MR updating a permissions matrix to be merged before merging an + MR from someone who hasn't yet been granted permissions + +It is common for a single logical change to span several merge requests. These +MRs may all be in a single project, or they may be spread out across multiple +projects, and the order in which they are merged can be significant. + +For example, given a project `mycorp/awesome-project` that imports a library +at `myfriend/awesome-lib`, adding a feature in `awesome-project` may **also** +require changes to `awesome-lib`, and so necessitate two merge requests. Merging +the `awesome-project` MR before the `awesome-lib` one would break the `master` +branch. + +The `awesome-project` MR could be [marked as WIP](work_in_progress_merge_requests.md), +and the reason for the WIP stated included in the comments. However, this +requires the state of the `awesome-lib` MR to be manually tracked, and doesn't +scale well if the `awesome-project` MR depends on changes to **several** other +projects. + +By marking the `awesome-project` MR as blocked on the `awesome-lib` MR instead, +the status of the dependency is automatically tracked by GitLab, and the WIP +state can be used to communicate the readiness of the code in each individual +MR instead. + +## Configuration + +To continue the above example, you can configure a block when creating the +new MR in `awesome-project` (or by editing it, if it already exists). The block +needs to be configured on the MR that will be **blocked**, rather than on the +**blocking** MR. There is a "Blocking merge requests" section in the form: + +![Blocking merge requests form control](img/edit_blocking_merge_requests.png) + +Anyone who can edit a merge request can change the list of blocking merge +requests. + +New blocks can be added by reference, by URL, or by using autcompletion. To +remove a block, press the "X" by its reference. + +As blocks can be specified across projects, it's possible that someone else has +added a block for a merge request in a project you don't have access to. These +are shown as a simple count: + +![Blocking merge requests form control with inaccessible MRs](img/edit_blocking_merge_requests_inaccessible.png) + +If necessary, you can remove all the blocks like this by pressing the "X", just +as you would for a single, visible block. + +Once you're finished, press the "Save changes" button to submit the request, or +"Cancel" to return without making any changes. + +The list of configured blocks, and the status of each one, is shown in the merge +request widget: + +![Blocking merge requests in merge request widget](img/show_blocking_merge_requests_in_mr_widget.png) + +Until all blocking merge requests have, themselves, been merged, the "Merge" +button will be disabled. In particular, note that **closed** merge requests +still block their dependents - it is impossible to automatically determine if +merge requests that were blocked by that MR when it was open, are still blocked +when it is closed. + +If a merge request has been closed **and** the block is no longer relevant, it +must be removed as a blocking MR, following the instructions above, before +merge. + +## Limitations + +* API support: [gitlab-ee#12551](https://gitlab.com/gitlab-org/gitlab-ee/issues/12551) +* Blocking relationships are not preserved across project export/import: [gitlab-ee#12549](https://gitlab.com/gitlab-org/gitlab-ee/issues/12549) +* Complex merge order dependencies are not supported: [gitlab-ee#11393](https://gitlab.com/gitlab-org/gitlab-ee/issues/11393) + +The last item merits a little more explanation. Blocking merge requests can be +described as a graph of dependencies. The simplest possible graph has one +merge request blocking another: + +```mermaid +graph LR; + myfriend/awesome-lib!10-->mycorp/awesome-project!100; +``` + +A more complex (and still supported) graph might have several MRs blocking +another from being merged: + +```mermaid +graph LR; + myfriend/awesome-lib!10-->mycorp/awesome-project!100; + herfriend/another-lib!1-->mycorp/awesome-project!100; +``` + +We also support one MR blocking several others from being merged: + +```mermaid +graph LR; + herfriend/another-lib!1-->myfriend/awesome-lib!10; + herfriend/another-lib!1-->mycorp/awesome-project!100; +``` + +What is **not** supported is a "deep", or "nested" graph of dependencies, e.g.: + +```mermaid +graph LR; + herfriend/another-lib!1-->myfriend/awesome-lib!10; + myfriend/awesome-lib!10-->mycorp/awesome-project!100; +``` + +In this example, `myfriend/awesome-lib!10` would be blocked from being merged by +`herfriend/another-lib!1`, and would also block `mycorp/awesome-project!100` +from being merged. This is **not** yet supported. + diff --git a/doc/user/project/merge_requests/img/edit_blocking_merge_requests.png b/doc/user/project/merge_requests/img/edit_blocking_merge_requests.png new file mode 100644 index 0000000000000000000000000000000000000000..0fe26d602e38ee508437f676908c710e27168dde Binary files /dev/null and b/doc/user/project/merge_requests/img/edit_blocking_merge_requests.png differ diff --git a/doc/user/project/merge_requests/img/edit_blocking_merge_requests_inaccessible.png b/doc/user/project/merge_requests/img/edit_blocking_merge_requests_inaccessible.png new file mode 100644 index 0000000000000000000000000000000000000000..a1003c41c2285809a1c49f653ccdfd46f78e7ffb Binary files /dev/null and b/doc/user/project/merge_requests/img/edit_blocking_merge_requests_inaccessible.png differ diff --git a/doc/user/project/merge_requests/img/show_blocking_merge_requests_in_mr_widget.png b/doc/user/project/merge_requests/img/show_blocking_merge_requests_in_mr_widget.png new file mode 100644 index 0000000000000000000000000000000000000000..a1241f9e38c37e31adf9b801f1048cff7149d7c3 Binary files /dev/null and b/doc/user/project/merge_requests/img/show_blocking_merge_requests_in_mr_widget.png differ diff --git a/doc/user/project/merge_requests/index.md b/doc/user/project/merge_requests/index.md index 8c28e24683f0c56f4bb9ae87515cdb79b5bdd496..08a5d2e03a396c0cead8818fd44185d336eac6cb 100644 --- a/doc/user/project/merge_requests/index.md +++ b/doc/user/project/merge_requests/index.md @@ -47,6 +47,7 @@ With **[GitLab Enterprise Edition][ee]**, you can also: - Analyze your dependencies for vulnerabilities with [Dependency Scanning](../../application_security/dependency_scanning/index.md) **(ULTIMATE)** - Analyze your Docker images for vulnerabilities with [Container Scanning](../../application_security/container_scanning/index.md) **(ULTIMATE)** - Determine the performance impact of changes with [Browser Performance Testing](#browser-performance-testing-premium) **(PREMIUM)** +- Specify merge order dependencies with [Blocking Merge Requests](#blocking-merge-requests-premium) **(PREMIUM)** ## Use cases @@ -412,6 +413,21 @@ GitLab runs the [Sitespeed.io container][sitespeed-container] and displays the d [Read more about Browser Performance Testing.](browser_performance_testing.md) +## Blocking Merge Requests **(PREMIUM)** + +> Introduced in [GitLab Premium][products] 12.2. + +A single logical change may be split across several merge requests, and perhaps +even across several projects. When this happens, the order in which MRs are +merged is important. + +GitLab allows you to specify that a merge request is blocked by other MRs. With +this relationship in place, the merge request cannot be merged until all of its +blockers have also been merged, helping to maintain the consistency of a single +logical change. + +[Read more about Blocking Merge Requests.](blocking_merge_requests.md) + ## Security reports **(ULTIMATE)** GitLab can scan and report any vulnerabilities found in your project. diff --git a/ee/app/assets/javascripts/pages/projects/merge_requests/shared/init_form.js b/ee/app/assets/javascripts/pages/projects/merge_requests/shared/init_form.js index 9a5adf1e184cf0c888cb4dac9a3ebfb5bcf38707..755746f74ba9a65c803d866a9a95819b7a5d8d98 100644 --- a/ee/app/assets/javascripts/pages/projects/merge_requests/shared/init_form.js +++ b/ee/app/assets/javascripts/pages/projects/merge_requests/shared/init_form.js @@ -1,5 +1,7 @@ import mountApprovals from 'ee/approvals/mount_mr_edit'; +import mountBlockingMergeRequestsInput from 'ee/projects/merge_requests/blocking_mr_input'; export default () => { mountApprovals(document.getElementById('js-mr-approvals-input')); + mountBlockingMergeRequestsInput(document.getElementById('js-blocking-merge-requests-input')); }; diff --git a/ee/app/assets/javascripts/projects/merge_requests/blocking_mr_input.js b/ee/app/assets/javascripts/projects/merge_requests/blocking_mr_input.js new file mode 100644 index 0000000000000000000000000000000000000000..af479f27e3ee90ad790fe358aa92c3cad5b07c2d --- /dev/null +++ b/ee/app/assets/javascripts/projects/merge_requests/blocking_mr_input.js @@ -0,0 +1,38 @@ +import Vue from 'vue'; +import BlockingMrInput from 'ee/projects/merge_requests/blocking_mr_input_root.vue'; +import { n__ } from '~/locale'; + +export default el => { + if (!el) { + return null; + } + const { hiddenBlockingMrsCount, visibleBlockingMrRefs } = el.dataset; + const parsedVisibleBlockingMrRefs = JSON.parse(visibleBlockingMrRefs); + const containsHiddenBlockingMrs = hiddenBlockingMrsCount > 0; + + const references = containsHiddenBlockingMrs + ? [ + ...parsedVisibleBlockingMrRefs, + { + text: n__( + '%d inaccessible merge request', + '%d inaccessible merge requests', + hiddenBlockingMrsCount, + ), + isHiddenRef: true, + }, + ] + : parsedVisibleBlockingMrRefs; + + return new Vue({ + el, + render(h) { + return h(BlockingMrInput, { + props: { + existingRefs: references, + containsHiddenBlockingMrs, + }, + }); + }, + }); +}; diff --git a/ee/app/assets/javascripts/projects/merge_requests/blocking_mr_input_root.vue b/ee/app/assets/javascripts/projects/merge_requests/blocking_mr_input_root.vue new file mode 100644 index 0000000000000000000000000000000000000000..9979d84dd87225c9cba360973a17a67d6f3dd616 --- /dev/null +++ b/ee/app/assets/javascripts/projects/merge_requests/blocking_mr_input_root.vue @@ -0,0 +1,105 @@ + + + diff --git a/ee/app/assets/javascripts/related_issues/components/related_issuable_input.vue b/ee/app/assets/javascripts/related_issues/components/related_issuable_input.vue index bdaa8311ad17a5086b4e2c03a65e68094bf650ce..c7256da97a48ba1cf97685c22a380d66dd51cd70 100644 --- a/ee/app/assets/javascripts/related_issues/components/related_issuable_input.vue +++ b/ee/app/assets/javascripts/related_issues/components/related_issuable_input.vue @@ -12,6 +12,11 @@ export default { issueToken, }, props: { + inputId: { + type: String, + required: false, + default: '', + }, references: { type: Array, required: false, @@ -162,7 +167,7 @@ export default { >
  • '), [issuableTypesMap.EPIC]: __(' or <#epic id>'), + [issuableTypesMap.MERGE_REQUEST]: __(' or <#merge request id>'), }, false: { [issuableTypesMap.ISSUE]: '', [issuableTypesMap.EPIC]: '', + [issuableTypesMap.MERGE_REQUEST]: '', }, }; export const inputPlaceholderTextMap = { [issuableTypesMap.ISSUE]: __('Paste issue link'), [issuableTypesMap.EPIC]: __('Paste epic link'), + [issuableTypesMap.MERGE_REQUEST]: __('Paste a merge request link'), }; export const relatedIssuesRemoveErrorMap = { diff --git a/ee/app/controllers/ee/projects/merge_requests/application_controller.rb b/ee/app/controllers/ee/projects/merge_requests/application_controller.rb index 4ca80117f3d1038100577e70c06ec6d8be772a26..e84f7c29a4430356efd89f7ddb0b931211d3ca01 100644 --- a/ee/app/controllers/ee/projects/merge_requests/application_controller.rb +++ b/ee/app/controllers/ee/projects/merge_requests/application_controller.rb @@ -23,6 +23,9 @@ def merge_request_params def merge_request_params_attributes attrs = super.push( + { blocking_merge_request_references: [] }, + :update_blocking_merge_request_refs, + :remove_hidden_blocking_merge_requests, approval_rule_attributes, :approvals_before_merge, :approver_group_ids, diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index 25a1a1d984923737af38587a0d79ec231f478581..09605a849af63adc141070bd29745bed9d904f8b 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -6,6 +6,7 @@ module MergeRequest extend ::Gitlab::Utils::Override include ::Approvable + include ::Gitlab::Allowable include ::Gitlab::Utils::StrongMemoize include FromUnion @@ -83,6 +84,28 @@ def supports_weight? false end + def visible_blocking_merge_requests(user) + Ability.merge_requests_readable_by_user(blocking_merge_requests, user) + end + + def visible_blocking_merge_request_refs(user) + visible_blocking_merge_requests(user).map do |mr| + mr.to_reference(target_project) + end + end + + # Unlike +visible_blocking_merge_requests+, this method doesn't include + # blocking MRs that have been merged. This simplifies output, since we don't + # need to tell the user that there are X hidden blocking MRs, of which only + # Y are an obstacle. Pass include_merged: true to override this behaviour. + def hidden_blocking_merge_requests_count(user, include_merged: false) + hidden = blocking_merge_requests - visible_blocking_merge_requests(user) + + hidden.delete_if(&:merged?) unless include_merged + + hidden.count + end + def validate_approval_rule_source return unless approval_rules.any? diff --git a/ee/app/models/merge_request_block.rb b/ee/app/models/merge_request_block.rb index 7432a05710d1a88dc45b9c7a0c82ec64fbf296d5..691ffb7ed97954d403f8c693e3f60cb8f65b738a 100644 --- a/ee/app/models/merge_request_block.rb +++ b/ee/app/models/merge_request_block.rb @@ -10,6 +10,27 @@ class MergeRequestBlock < ApplicationRecord validate :check_block_constraints + scope :with_blocking_mr_ids, -> (ids) do + where(blocking_merge_request_id: ids).includes(:blocking_merge_request) + end + + # Add a number of blocks at once. Pass a hash of blocking_id => blocked_id, or + # an Array of [blocking_id, blocked_id] tuples + def self.bulk_insert(pairs) + now = Time.current + + rows = pairs.map do |blocking, blocked| + { + blocking_merge_request_id: blocking, + blocked_merge_request_id: blocked, + created_at: now, + updated_at: now + } + end + + ::Gitlab::Database.bulk_insert(table_name, rows) + end + private def check_block_constraints diff --git a/ee/app/serializers/ee/merge_request_widget_entity.rb b/ee/app/serializers/ee/merge_request_widget_entity.rb index 8d46aeb84f49adc2fcf283270839e5038143714d..9c18dafabd207e193f328a87f8ec4d5720fe6a68 100644 --- a/ee/app/serializers/ee/merge_request_widget_entity.rb +++ b/ee/app/serializers/ee/merge_request_widget_entity.rb @@ -173,21 +173,14 @@ module MergeRequestWidgetEntity private def blocking_merge_requests - visible_mrs_by_state = Hash.new { |h, k| h[k] = [] } - visible_count = 0 - hidden_blocking_count = 0 - - object.blocking_merge_requests.each do |mr| - if can?(current_user, :read_merge_request, mr) - visible_mrs_by_state[mr.state_name] << represent_blocking_mr(mr) - visible_count += 1 - elsif !mr.merged? # Ignore merged hidden MRs to make display simpler - hidden_blocking_count += 1 - end - end + hidden_blocking_count = object.hidden_blocking_merge_requests_count(current_user) + visible_mrs = object.visible_blocking_merge_requests(current_user) + visible_mrs_by_state = visible_mrs + .map { |mr| represent_blocking_mr(mr) } + .group_by { |blocking_mr| blocking_mr.object.state_name } { - total_count: visible_count + hidden_blocking_count, + total_count: visible_mrs.count + hidden_blocking_count, hidden_count: hidden_blocking_count, visible_merge_requests: visible_mrs_by_state } diff --git a/ee/app/services/ee/merge_requests/base_service.rb b/ee/app/services/ee/merge_requests/base_service.rb index 3e9b7a7abea1c7ca2c6c4c8e2e6c956b8315fdb5..5f96392f895de18743468cf728b5eefbff5a314e 100644 --- a/ee/app/services/ee/merge_requests/base_service.rb +++ b/ee/app/services/ee/merge_requests/base_service.rb @@ -5,6 +5,8 @@ module MergeRequests module BaseService private + attr_accessor :blocking_merge_requests_params + def filter_params(merge_request) unless current_user.can?(:update_approvers, merge_request) params.delete(:approvals_before_merge) @@ -14,6 +16,9 @@ def filter_params(merge_request) self.params = ApprovalRules::ParamsFilteringService.new(merge_request, current_user, params).execute + self.blocking_merge_requests_params = + ::MergeRequests::UpdateBlocksService.extract_params!(params) + super end end diff --git a/ee/app/services/ee/merge_requests/create_service.rb b/ee/app/services/ee/merge_requests/create_service.rb index c151cf76ad4364acc7b8eb7e91af346eb23c7334..45b535c4310350ec366d9a24ebcd5096ed479f85 100644 --- a/ee/app/services/ee/merge_requests/create_service.rb +++ b/ee/app/services/ee/merge_requests/create_service.rb @@ -17,6 +17,10 @@ def after_create(issuable) if pipeline ::SyncSecurityReportsToReportApprovalRulesWorker.perform_async(pipeline.id) end + + ::MergeRequests::UpdateBlocksService + .new(issuable, current_user, blocking_merge_requests_params) + .execute end end end diff --git a/ee/app/services/ee/merge_requests/update_service.rb b/ee/app/services/ee/merge_requests/update_service.rb index 8e8a6c2de0c2c4cd11a0fbcf448c007a48657029..f471e43965acacb1fdb29a638790b03148d07c57 100644 --- a/ee/app/services/ee/merge_requests/update_service.rb +++ b/ee/app/services/ee/merge_requests/update_service.rb @@ -31,6 +31,10 @@ def execute(merge_request) notification_service.add_merge_request_approvers(merge_request, new_approvers, current_user) end + ::MergeRequests::UpdateBlocksService + .new(merge_request, current_user, blocking_merge_requests_params) + .execute + merge_request end diff --git a/ee/app/services/merge_requests/update_blocks_service.rb b/ee/app/services/merge_requests/update_blocks_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..fdc3afb3cdb7e91bd532b70e71be52215336c854 --- /dev/null +++ b/ee/app/services/merge_requests/update_blocks_service.rb @@ -0,0 +1,96 @@ +# frozen_string_literal: true + +module MergeRequests + class UpdateBlocksService + include ::Gitlab::Allowable + include ::Gitlab::Utils::StrongMemoize + + class << self + def extract_params!(mutable_params) + { + update: mutable_params.delete(:update_blocking_merge_request_refs), + remove_hidden: mutable_params.delete(:remove_hidden_blocking_merge_requests), + references: mutable_params.delete(:blocking_merge_request_references) + } + end + end + + attr_reader :merge_request, :current_user, :params + + def initialize(merge_request, current_user, params = {}) + @merge_request = merge_request + @current_user = current_user + @params = params + + DeclarativePolicy.user_scope do + @visible_blocks, @hidden_blocks = merge_request.blocks_as_blockee.partition do |block| + can?(current_user, :read_merge_request, block.blocking_merge_request) + end + end + end + + def execute + return unless update? + return unless merge_request.target_project.feature_available?(:blocking_merge_requests) + + merge_request + .blocks_as_blockee + .with_blocking_mr_ids(ids_to_del) + .delete_all + + ::MergeRequestBlock.bulk_insert( + ids_to_add.map { |blocking_id| [blocking_id, merge_request.id] } + ) + + true + end + + private + + attr_reader :visible_blocks, :hidden_blocks + + def update? + params.fetch(:update, false) + end + + def remove_hidden? + params.fetch(:remove_hidden, false) + end + + def references + params.fetch(:references, []) + end + + def requested_ids + strong_memoize(:requested_ids) do + next [] unless references.present? + + # The analyzer will only return references the current user can see + analyzer = ::Gitlab::ReferenceExtractor.new(merge_request.target_project, current_user) + analyzer.analyze(references.join(" ")) + + analyzer.merge_requests.map(&:id) + end + end + + def visible_ids + strong_memoize(:visible_ids) { visible_blocks.map(&:blocking_merge_request_id) } + end + + def hidden_ids + strong_memoize(:hidden_ids) { hidden_blocks.map(&:blocking_merge_request_id) } + end + + def ids_to_add + strong_memoize(:ids_to_add) { requested_ids - visible_ids } + end + + def ids_to_del + strong_memoize(:ids_to_del) do + (visible_ids - requested_ids).tap do |ary| + ary.push(*hidden_ids) if remove_hidden? + end + end + end + end +end diff --git a/ee/app/views/shared/issuable/form/_merge_request_blocks.html.haml b/ee/app/views/shared/issuable/form/_merge_request_blocks.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..13677a7ca0523ed8cd1339f644f221010adadb1a --- /dev/null +++ b/ee/app/views/shared/issuable/form/_merge_request_blocks.html.haml @@ -0,0 +1,17 @@ +- merge_request = local_assigns.fetch(:issuable) + +- return unless merge_request.is_a?(MergeRequest) + +- form = local_assigns.fetch(:form) +- project = merge_request.target_project + +- return unless project&.feature_available?(:blocking_merge_requests) + +.form-group.row.blocking-merge-requests + = form.label :blocking_merge_request_references, _('Blocking merge requests'), class: 'col-form-label col-sm-2' + .col-sm-10 + = text_field_tag 'blocking_merge_request_refs', nil, + class: "form-control ", + id: "js-blocking-merge-requests-input", + data: { hidden_blocking_mrs_count: merge_request.hidden_blocking_merge_requests_count(current_user), + visible_blocking_mr_refs: merge_request.visible_blocking_merge_request_refs(current_user) } diff --git a/ee/changelogs/unreleased/9688-add-remove-blocking-mrs.yml b/ee/changelogs/unreleased/9688-add-remove-blocking-mrs.yml new file mode 100644 index 0000000000000000000000000000000000000000..f8120dac0cd6e4a4cb08f2172318599a9d49cbdb --- /dev/null +++ b/ee/changelogs/unreleased/9688-add-remove-blocking-mrs.yml @@ -0,0 +1,5 @@ +--- +title: Support for blocking merge requests +merge_request: 13506 +author: +type: added diff --git a/ee/spec/features/merge_request/user_creates_merge_request_with_blocking_mrs_spec.rb b/ee/spec/features/merge_request/user_creates_merge_request_with_blocking_mrs_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..3bfd1e1430ed63ad8d0230b5ced9853eb7568d32 --- /dev/null +++ b/ee/spec/features/merge_request/user_creates_merge_request_with_blocking_mrs_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'User creates a merge request with blocking MRs', :js do + let(:project) { create(:project, :repository) } + let(:user) { project.owner } + + let(:mr_params) { { title: 'Some feature', source_branch: 'fix', target_branch: 'feature' } } + + before do + sign_in(user) + end + + context 'feature is enabled' do + before do + stub_licensed_features(blocking_merge_requests: true) + end + + it 'creates a merge request with a blocking MR' do + other_mr = create(:merge_request) + other_mr.target_project.team.add_maintainer(user) + + visit(project_new_merge_request_path(project, merge_request: mr_params)) + + fill_in 'Blocking merge requests', with: other_mr.to_reference(full: true) + click_button('Submit merge request') + + expect(page).to have_content('Blocked by 1 merge request') + end + end + + context 'feature is disabled' do + before do + stub_licensed_features(blocking_merge_requests: false) + end + + it 'does not show blocking MRs controls' do + visit(project_new_merge_request_path(project, merge_request: mr_params)) + + expect(page).not_to have_content('Blocking merge requests') + end + end +end diff --git a/ee/spec/features/merge_request/user_edits_merge_request_blocking_mrs_spec.rb b/ee/spec/features/merge_request/user_edits_merge_request_blocking_mrs_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..cad8cc5ebde3c3535bfbb90e27cb040e8a6cdf0f --- /dev/null +++ b/ee/spec/features/merge_request/user_edits_merge_request_blocking_mrs_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe "User edits merge request with blocking MRs", :js do + let(:merge_request) { create(:merge_request) } + let(:project) { merge_request.target_project } + let(:user) { merge_request.target_project.owner } + + let(:other_mr) { create(:merge_request) } + + before do + sign_in(user) + end + + context 'feature is enabled' do + before do + stub_licensed_features(blocking_merge_requests: true) + end + + context 'user can see the other MR' do + before do + other_mr.target_project.team.add_developer(user) + end + + it 'can add the other MR' do + visit edit_project_merge_request_path(project, merge_request) + + fill_in 'Blocking merge requests', with: other_mr.to_reference(full: true) + + click_button 'Save changes' + + expect(page).to have_content('Blocked by 1 merge request') + end + + it 'can see and remove an existing blocking MR' do + create(:merge_request_block, blocking_merge_request: other_mr, blocked_merge_request: merge_request) + + visit edit_project_merge_request_path(project, merge_request) + + expect(page).to have_content(other_mr.to_reference(full: true)) + + click_button "Remove #{other_mr.to_reference(full: true)}" + click_button 'Save changes' + + expect(page).not_to have_content('Blocked by 1 merge request') + expect(page).not_to have_content(other_mr.to_reference(full: true)) + end + end + + context 'user cannot see the other MR' do + it 'cannot add the other MR' do + visit edit_project_merge_request_path(project, merge_request) + + fill_in 'Blocking merge requests', with: other_mr.to_reference(full: true) + + click_button 'Save changes' + + expect(page).not_to have_content('Blocked by 1 merge request') + end + + it 'sees the existing MR as hidden and can remove it' do + create(:merge_request_block, blocking_merge_request: other_mr, blocked_merge_request: merge_request) + + visit edit_project_merge_request_path(project, merge_request) + + expect(page).to have_content('1 inaccessible merge request') + + click_button 'Remove 1 inaccessible merge request' + click_button 'Save changes' + + expect(page).not_to have_content('Blocked by 1 merge request') + expect(page).not_to have_content(other_mr.to_reference(full: true)) + end + end + end + + context 'feature is disabled' do + before do + stub_licensed_features(blocking_merge_requests: false) + end + + it 'cannot see the blocking MR controls' do + visit edit_project_merge_request_path(project, merge_request) + + expect(page).not_to have_content('Blocking merge requests') + end + end +end diff --git a/ee/spec/frontend/projects/merge_requests/blocking_mr_input_root_spec.js b/ee/spec/frontend/projects/merge_requests/blocking_mr_input_root_spec.js new file mode 100644 index 0000000000000000000000000000000000000000..45dae3d6dcf81504fc3f215044790c40ec9a54a0 --- /dev/null +++ b/ee/spec/frontend/projects/merge_requests/blocking_mr_input_root_spec.js @@ -0,0 +1,136 @@ +import { shallowMount } from '@vue/test-utils'; +import BlockingMrInputRoot from 'ee/projects/merge_requests/blocking_mr_input_root.vue'; +import RelatedIssuableInput from 'ee/related_issues/components/related_issuable_input.vue'; + +describe('blocking mr input root', () => { + let wrapper; + + const getInput = () => wrapper.find(RelatedIssuableInput); + const addTokenizedInput = input => { + getInput().vm.$emit('addIssuableFormInput', { + untouchedRawReferences: [input], + touchedReference: '', + }); + }; + const addInput = input => { + getInput().vm.$emit('addIssuableFormInput', { + untouchedRawReferences: [], + touchedReference: input, + }); + }; + const removeRef = index => { + getInput().vm.$emit('pendingIssuableRemoveRequest', index); + }; + const createComponent = (propsData = {}) => { + wrapper = shallowMount(BlockingMrInputRoot, { propsData }); + }; + + it('does not keep duplicate references', () => { + createComponent(); + const input = '!1'; + + addTokenizedInput(input); + addTokenizedInput(input); + + expect(wrapper.vm.references).toEqual(['!1']); + }); + + it('updates input value to empty string when adding a tokenized input', () => { + createComponent(); + + addTokenizedInput('foo'); + + expect(wrapper.vm.inputValue).toBe(''); + }); + + it('updates input value to ref when typing into input (before adding whitespace)', () => { + createComponent(); + + addInput('foo'); + + expect(wrapper.vm.inputValue).toBe('foo'); + }); + + it('does not reorder when adding a ref that already exists', () => { + const input = '!1'; + createComponent({ + existingRefs: [input, '!2'], + }); + + addTokenizedInput(input, wrapper); + + expect(wrapper.vm.references).toEqual(['!1', '!2']); + }); + + it('does not add empty reference on blur', () => { + createComponent(); + + getInput().vm.$emit('addIssuableFormBlur', ''); + + expect(wrapper.vm.references).toHaveLength(0); + }); + + describe('hidden inputs', () => { + const createHiddenInputExpectation = selector => bool => { + expect(wrapper.find(selector).element.value).toBe(`${bool}`); + }; + + describe('update_blocking_merge_request_refs', () => { + const expectShouldUpdateRefsToBe = createHiddenInputExpectation( + 'input[name="merge_request[update_blocking_merge_request_refs]"]', + ); + + it('is false when nothing happens', () => { + createComponent(); + + expectShouldUpdateRefsToBe(false); + }); + + it('is true after a ref is removed', () => { + createComponent({ existingRefs: ['!1'] }); + removeRef(0); + + expectShouldUpdateRefsToBe(true); + }); + + it('is true after a ref is added', () => { + createComponent(); + addTokenizedInput('foo'); + + expectShouldUpdateRefsToBe(true); + }); + }); + + describe('remove_hidden_blocking_merge_requests', () => { + const expectRemoveHiddenBlockingMergeRequestsToBe = createHiddenInputExpectation( + 'input[name="merge_request[update_blocking_merge_request_refs]"]', + ); + const makeComponentWithHiddenMrs = () => { + const hiddenMrsRef = '2 inaccessible merge requests'; + createComponent({ + containsHiddenBlockingMrs: true, + existingRefs: ['!1', '!2', hiddenMrsRef], + }); + }; + + it('is true when nothing has happened', () => { + makeComponentWithHiddenMrs(); + + expectRemoveHiddenBlockingMergeRequestsToBe(false); + }); + + it('is false when removing any other MRs', () => { + makeComponentWithHiddenMrs(); + + expectRemoveHiddenBlockingMergeRequestsToBe(false); + }); + + it('is false when ref has been removed', () => { + makeComponentWithHiddenMrs(); + removeRef(2); + + expectRemoveHiddenBlockingMergeRequestsToBe(true); + }); + }); + }); +}); diff --git a/ee/spec/frontend/projects/merge_requests/blocking_mr_input_spec.js b/ee/spec/frontend/projects/merge_requests/blocking_mr_input_spec.js new file mode 100644 index 0000000000000000000000000000000000000000..d23d11d15e7b80b844c75da45a5c104fd81e6aff --- /dev/null +++ b/ee/spec/frontend/projects/merge_requests/blocking_mr_input_spec.js @@ -0,0 +1,50 @@ +import Vue from 'vue'; +import initBlockingMrInput from 'ee/projects/merge_requests/blocking_mr_input'; + +jest.mock('vue'); + +describe('BlockingMrInput', () => { + let h; + const refs = ['!1']; + const getProps = () => h.mock.calls[0][1].props; + const callRender = () => { + Vue.mock.calls[0][0].render(h); + }; + const setInnerHtml = (visibleMrs = refs, hiddenCount = 2) => { + document.body.innerHTML += `
    `; + }; + + beforeEach(() => { + h = jest.fn(); + }); + + afterEach(() => { + document.querySelector('#test').remove(); + jest.clearAllMocks(); + }); + + it('adds hidden references block when hidden count is greater than 0', () => { + setInnerHtml(); + initBlockingMrInput(document.querySelector('#test')); + callRender(); + expect(getProps().existingRefs[refs.length].text).toBe('2 inaccessible merge requests'); + }); + + it('containsHiddenBlockingMrs is true when count is greater than one', () => { + setInnerHtml(); + initBlockingMrInput(document.querySelector('#test')); + + callRender(); + expect(getProps().containsHiddenBlockingMrs).toBe(true); + }); + + it('containsHiddenBlockingMrs is false when count is zero', () => { + setInnerHtml(refs, 0); + initBlockingMrInput(document.querySelector('#test')); + + callRender(); + expect(getProps().containsHiddenBlockingMrs).toBe(false); + }); +}); diff --git a/ee/spec/models/merge_request/blocking_spec.rb b/ee/spec/models/merge_request/blocking_spec.rb index ce9432d9e2e385df0d1cefe7b146c7a68c0c8c97..b01d1290ddb7a9ac98e6983aec1fdd25d393abb9 100644 --- a/ee/spec/models/merge_request/blocking_spec.rb +++ b/ee/spec/models/merge_request/blocking_spec.rb @@ -68,4 +68,80 @@ end end end + + describe '#visible_blocking_merge_requests' do + let(:block) { create(:merge_request_block) } + let(:blocking_mr) { block.blocking_merge_request } + let(:blocked_mr) { block.blocked_merge_request } + let(:user) { create(:user) } + + it 'shows blocking MR to developer' do + blocking_mr.target_project.team.add_developer(user) + + expect(blocked_mr.visible_blocking_merge_requests(user)).to contain_exactly(blocking_mr) + end + + it 'hides block from guest' do + blocking_mr.target_project.team.add_guest(user) + + expect(blocked_mr.visible_blocking_merge_requests(user)).to be_empty + end + + it 'hides block from anonymous user' do + expect(blocked_mr.visible_blocking_merge_requests(nil)).to be_empty + end + end + + describe '#visible_blocking_merge_request_refs' do + let(:merge_request) { create(:merge_request) } + let(:other_mr) { create(:merge_request) } + let(:user) { create(:user) } + + it 'returns the references for the result of #visible_blocking_merge_requests' do + expect(merge_request) + .to receive(:visible_blocking_merge_requests) + .with(user) + .and_return([other_mr]) + + expect(merge_request.visible_blocking_merge_request_refs(user)) + .to eq([other_mr.to_reference(full: true)]) + end + end + + describe '#hidden_blocking_merge_requests_count' do + let(:block) { create(:merge_request_block) } + let(:blocking_mr) { block.blocking_merge_request } + let(:blocked_mr) { block.blocked_merge_request } + let(:user) { create(:user) } + + it 'returns 0 when all MRs are visible' do + blocking_mr.target_project.team.add_developer(user) + + expect(blocked_mr.hidden_blocking_merge_requests_count(user)).to eq(0) + end + + context 'MR is hidden' do + before do + blocking_mr.target_project.team.add_guest(user) + end + + it 'returns 1 when MR is unmerged by default' do + expect(blocked_mr.hidden_blocking_merge_requests_count(user)).to eq(1) + end + + context 'MR is merged' do + before do + blocking_mr.update_columns(state: 'merged') + end + + it 'returns 0 by default' do + expect(blocked_mr.hidden_blocking_merge_requests_count(user)).to eq(0) + end + + it 'returns 1 when include_merged: true' do + expect(blocked_mr.hidden_blocking_merge_requests_count(user, include_merged: true)).to eq(1) + end + end + end + end end diff --git a/ee/spec/models/merge_request_block_spec.rb b/ee/spec/models/merge_request_block_spec.rb index 207be321f26b272f0ae4fae7e4e6418ab98e3043..78c76187ad82c508e4df8c0859ab6d14bee7f76d 100644 --- a/ee/spec/models/merge_request_block_spec.rb +++ b/ee/spec/models/merge_request_block_spec.rb @@ -60,4 +60,45 @@ expect(new_block).not_to be_valid end end + + describe '.bulk_insert' do + let(:mrs) { create_list(:merge_request, 4) } + + it 'inserts multiple blocks specified as a Hash' do + described_class.bulk_insert( + mrs[0].id => mrs[1].id, + mrs[2].id => mrs[3].id + ) + + expect(described_class.all).to contain_exactly( + having_attributes(blocking_merge_request_id: mrs[0].id, blocked_merge_request_id: mrs[1].id), + having_attributes(blocking_merge_request_id: mrs[2].id, blocked_merge_request_id: mrs[3].id) + ) + end + + it 'inserts multiple blocks specified as an Array' do + described_class.bulk_insert([[mrs[0].id, mrs[1].id], [mrs[2].id, mrs[3].id]]) + + expect(described_class.all).to contain_exactly( + having_attributes(blocking_merge_request_id: mrs[0].id, blocked_merge_request_id: mrs[1].id), + having_attributes(blocking_merge_request_id: mrs[2].id, blocked_merge_request_id: mrs[3].id) + ) + end + end + + describe '.with_blocking_mr_ids' do + let!(:block) { create(:merge_request_block) } + let!(:other_block) { create(:merge_request_block) } + + subject(:result) { described_class.with_blocking_mr_ids([block.blocking_merge_request_id]) } + + it 'returns blocks with a matching blocking_merge_request_id' do + is_expected.to contain_exactly(block) + end + + it 'eager-loads the blocking MRs' do + association = result.first.association(:blocking_merge_request) + expect(association.loaded?).to be(true) + end + end end diff --git a/ee/spec/services/merge_requests/create_service_spec.rb b/ee/spec/services/merge_requests/create_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..24ab451b080a0a8df88b86cb251f7f9a605b99ed --- /dev/null +++ b/ee/spec/services/merge_requests/create_service_spec.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe MergeRequests::CreateService do + let(:project) { create(:project, :repository) } + let(:user) { project.owner } + + subject(:service) { described_class.new(project, user, params) } + + describe '#execute' do + context 'with blocking merge requests' do + let(:params) { { title: 'Blocked MR', source_branch: 'feature', target_branch: 'master' } } + + it 'delegates to MergeRequests::UpdateBlocksService' do + expect(MergeRequests::UpdateBlocksService) + .to receive(:extract_params!) + .and_return(:extracted_params) + + expect_next_instance_of(MergeRequests::UpdateBlocksService) do |block_service| + expect(block_service.merge_request.title).to eq('Blocked MR') + expect(block_service.current_user).to eq(user) + expect(block_service.params).to eq(:extracted_params) + + expect(block_service).to receive(:execute) + end + + service.execute + end + end + end +end diff --git a/ee/spec/services/merge_requests/update_blocks_service_spec.rb b/ee/spec/services/merge_requests/update_blocks_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..a67e8bb2214388158cf809f36f6611fac57264be --- /dev/null +++ b/ee/spec/services/merge_requests/update_blocks_service_spec.rb @@ -0,0 +1,115 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe MergeRequests::UpdateBlocksService do + describe '.extract_params!' do + it 'removes and reformats merge request params' do + mr_params = { + unrelated: true, + update_blocking_merge_request_refs: true, + remove_hidden_blocking_merge_requests: true, + blocking_merge_request_references: ['!1'] + } + + block_params = described_class.extract_params!(mr_params) + + expect(block_params).to eq( + update: true, + remove_hidden: true, + references: ['!1'] + ) + + expect(mr_params).to eq(unrelated: true) + end + end + + describe '#execute' do + let(:merge_request) { create(:merge_request) } + let(:user) { merge_request.target_project.owner } + + let(:mr_to_ignore) { create(:merge_request) } + let(:mr_to_add) { create(:merge_request) } + let(:mr_to_keep) { create(:merge_request) } + let(:mr_to_del) { create(:merge_request) } + let(:hidden_mr) { create(:merge_request) } + + let(:refs) do + [mr_to_ignore, mr_to_add, mr_to_keep].map { |mr| mr.to_reference(full: true) } + end + + let(:params) do + { + remove_hidden: remove_hidden, + references: refs, + update: update + } + end + + subject(:service) { described_class.new(merge_request, user, params) } + + before do + [mr_to_add, mr_to_keep, mr_to_del].each do |mr| + mr.target_project.team.add_maintainer(user) + end + + create(:merge_request_block, blocking_merge_request: mr_to_keep, blocked_merge_request: merge_request) + create(:merge_request_block, blocking_merge_request: mr_to_del, blocked_merge_request: merge_request) + create(:merge_request_block, blocking_merge_request: hidden_mr, blocked_merge_request: merge_request) + end + + context 'licensed' do + before do + stub_licensed_features(blocking_merge_requests: true) + end + + context 'with update: false' do + let(:update) { false } + let(:remove_hidden) { true } + + it 'does nothing' do + expect { service.execute }.not_to change { MergeRequestBlock.count } + end + end + + context 'with update: true' do + let(:update) { true } + + context 'with remove_hidden: false' do + let(:remove_hidden) { false } + + it 'adds only the requested MRs the user can see' do + service.execute + + expect(merge_request.blocking_merge_requests) + .to contain_exactly(mr_to_add, mr_to_keep, hidden_mr) + end + end + + context 'with remove_hidden: true' do + let(:remove_hidden) { true } + + it 'adds visible MRs and removes the hidden MR' do + service.execute + + expect(merge_request.blocking_merge_requests) + .to contain_exactly(mr_to_add, mr_to_keep) + end + end + end + end + + context 'unlicensed' do + let(:update) { true } + let(:remove_hidden) { true } + + before do + stub_licensed_features(blocking_merge_requests: false) + end + + it 'does nothing' do + expect { service.execute }.not_to change { MergeRequestBlock.count } + end + end + end +end diff --git a/ee/spec/services/merge_requests/update_service_spec.rb b/ee/spec/services/merge_requests/update_service_spec.rb index 2034e43e13942ae40fdde47504b682b9c0e68e41..eea67ba15b545a505bdbcba0c67e08df384b84f5 100644 --- a/ee/spec/services/merge_requests/update_service_spec.rb +++ b/ee/spec/services/merge_requests/update_service_spec.rb @@ -179,5 +179,23 @@ def update_merge_request(opts) expect(merge_request.reload.approvals).to be_empty end end + + context 'updating blocking merge requests' do + it 'delegates to MergeRequests::UpdateBlocksService' do + expect(MergeRequests::UpdateBlocksService) + .to receive(:extract_params!) + .and_return(:extracted_params) + + expect_next_instance_of(MergeRequests::UpdateBlocksService) do |service| + expect(service.merge_request).to eq(merge_request) + expect(service.current_user).to eq(user) + expect(service.params).to eq(:extracted_params) + + expect(service).to receive(:execute) + end + + update_merge_request({}) + end + end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 396cd1340e4b357fb91aa7ca3e8ac1be2375394c..3644023b65a0d2244c25df76bb8cfdffe95c4601 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -56,6 +56,9 @@ msgstr "" msgid " or <#issue id>" msgstr "" +msgid " or <#merge request id>" +msgstr "" + msgid "%d comment" msgid_plural "%d comments" msgstr[0] "" @@ -99,6 +102,11 @@ msgid_plural "%d fixed test results" msgstr[0] "" msgstr[1] "" +msgid "%d inaccessible merge request" +msgid_plural "%d inaccessible merge requests" +msgstr[0] "" +msgstr[1] "" + msgid "%d issue" msgid_plural "%d issues" msgstr[0] "" @@ -2117,6 +2125,9 @@ msgid_plural "Blocked by %d closed merge requests." msgstr[0] "" msgstr[1] "" +msgid "Blocking merge requests" +msgstr "" + msgid "Blog" msgstr "" @@ -9750,6 +9761,9 @@ msgstr "" msgid "Paste a machine public key here. Read more about how to generate it %{link_start}here%{link_end}" msgstr "" +msgid "Paste a merge request link" +msgstr "" + msgid "Paste epic link" msgstr "" diff --git a/spec/features/projects/tree/create_directory_spec.rb b/spec/features/projects/tree/create_directory_spec.rb index 2cb2a23b7be81121f346ff699aaaad700a31076a..8585e24bc35059f78c219ad47a14bbb7ad416e69 100644 --- a/spec/features/projects/tree/create_directory_spec.rb +++ b/spec/features/projects/tree/create_directory_spec.rb @@ -47,7 +47,9 @@ fill_in('commit-message', with: 'commit message ide') - click_button('Commit') + page.within '.multi-file-commit-form' do + click_button('Commit') + end find('.js-ide-edit-mode').click diff --git a/spec/features/projects/tree/create_file_spec.rb b/spec/features/projects/tree/create_file_spec.rb index 9f5524da8e9b37475223abba0b690cbb9691123b..8623b10562d38f483f430486e2b705139b768dc5 100644 --- a/spec/features/projects/tree/create_file_spec.rb +++ b/spec/features/projects/tree/create_file_spec.rb @@ -39,7 +39,9 @@ fill_in('commit-message', with: 'commit message ide') - click_button('Commit') + page.within '.multi-file-commit-form' do + click_button('Commit') + end expect(page).to have_content('file name') end diff --git a/spec/support/capybara.rb b/spec/support/capybara.rb index 56ac208a025ed23b5d345921d8fb5a4511bb51f1..60990879fe2cc308158a2541ac2963df656197b2 100644 --- a/spec/support/capybara.rb +++ b/spec/support/capybara.rb @@ -58,6 +58,7 @@ Capybara.default_max_wait_time = timeout Capybara.ignore_hidden_elements = true Capybara.default_normalize_ws = true +Capybara.enable_aria_label = true # Keep only the screenshots generated from the last failing test suite Capybara::Screenshot.prune_strategy = :keep_last_run