Merge branch 'ml-add-codeowner-approve-and-merge' into 'master'

Add an E2E test of code owner approval and merge

Closes gitlab-org/quality/testcases#803

See merge request gitlab-org/gitlab!31180

ee/app/assets/javascripts/approvals/components/mr_edit/rule_input.vue

@@ -47,6 +47,7 @@ export default {
     class="form-control mw-6em"
     type="number"
     :min="rule.minApprovalsRequired || 0"
+    data-qa-selector="approvals_number_field"
     @input="onInputChange"
   />
 </template>

ee/app/views/projects/_merge_request_approvals_settings.html.haml

 - return unless @project.feature_available?(:merge_requests, current_user)
 - return unless @project.feature_available?(:merge_request_approvers, current_user)
 
-%section.qa-merge-request-approval-settings.settings.merge-requests-feature.no-animate#js-merge-request-approval-settings{ class: [('expanded' if expanded)] }
+%section.settings.merge-requests-feature.no-animate#js-merge-request-approval-settings{ class: [('expanded' if expanded)], data: { qa_selector: 'merge_request_approvals_settings' } }
   .settings-header
     %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only= _("Merge request approvals")
     %button.btn.js-settings-toggle{ type: 'button' }= expanded ? _("Collapse") : _("Expand")

ee/app/views/projects/protected_branches/ee/_code_owner_approval_table.html.haml

 - if @project.feature_available?(:code_owner_approval_required)
   %td
-    = render "shared/buttons/project_feature_toggle", is_checked: protected_branch.code_owner_approval_required, label: s_("ProtectedBranch|Toggle code owner approval"), class_list: "js-code-owner-toggle project-feature-toggle mr-5"
+    = render "shared/buttons/project_feature_toggle", is_checked: protected_branch.code_owner_approval_required, label: s_("ProtectedBranch|Toggle code owner approval"), class_list: "js-code-owner-toggle project-feature-toggle mr-5", data: { qa_selector: 'code_owner_toggle_button', qa_branch_name: protected_branch.name }

qa/qa/ee.rb

@@ -116,13 +116,15 @@ module QA
 
         module Settings
           autoload :ProtectedBranches, 'qa/ee/page/project/settings/protected_branches'
+          autoload :Main, 'qa/ee/page/project/settings/main'
           autoload :MirroringRepositories, 'qa/ee/page/project/settings/mirroring_repositories'
           autoload :MergeRequest, 'qa/ee/page/project/settings/merge_request'
+          autoload :MergeRequestApprovals, 'qa/ee/page/project/settings/merge_request_approvals'
           autoload :Integrations, 'qa/ee/page/project/settings/integrations'
           autoload :Repository, 'qa/ee/page/project/settings/repository'
           autoload :PushRules, 'qa/ee/page/project/settings/push_rules'
-          autoload :CICD, 'qa/ee/page/project/settings/ci_cd.rb'
-          autoload :LicenseCompliance, 'qa/ee/page/project/settings/license_compliance.rb'
+          autoload :CICD, 'qa/ee/page/project/settings/ci_cd'
+          autoload :LicenseCompliance, 'qa/ee/page/project/settings/license_compliance'
 
           module Services
             autoload :Jenkins, 'qa/ee/page/project/settings/services/jenkins'

qa/qa/ee/page/merge_request/show.rb

@@ -7,6 +7,8 @@ module QA
         module Show
           extend QA::Page::PageConcern
 
+          ApprovalConditionsError = Class.new(RuntimeError)
+
           def self.prepended(base)
             super
 
@@ -99,7 +101,10 @@ module QA
           end
 
           def approvals_required_from
-            approvals_content.match(/approvals? from (.*)/)[1]
+            match = approvals_content.match(/approvals? from (.*)/)
+            raise(ApprovalConditionsError, 'The expected approval conditions were not found.') unless match
+
+            match[1]
           end
 
           def approved?
@@ -293,7 +298,7 @@ module QA
           end
 
           def merge_via_merge_train
-            raise ElementNotFound, "Not ready to merge" unless ready_to_merge?
+            wait_until_ready_to_merge
 
             click_element(:merge_button, text: "Start merge train")
 

qa/qa/ee/page/project/settings/main.rb

+# frozen_string_literal: true
+
+module QA
+  module EE
+    module Page
+      module Project
+        module Settings
+          module Main
+            extend QA::Page::PageConcern
+
+            def self.prepended(base)
+              super
+
+              base.class_eval do
+                view 'ee/app/views/projects/_merge_request_approvals_settings.html.haml' do
+                  element :merge_request_approvals_settings
+                end
+              end
+            end
+
+            def expand_merge_request_approvals_settings(&block)
+              expand_section(:merge_request_approvals_settings) do
+                MergeRequestApprovals.perform(&block)
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end

qa/qa/ee/page/project/settings/merge_request_approvals.rb

+# frozen_string_literal: true
+
+module QA
+  module EE
+    module Page
+      module Project
+        module Settings
+          class MergeRequestApprovals < QA::Page::Base
+            view 'ee/app/assets/javascripts/approvals/components/mr_edit/rule_input.vue' do
+              element :approvals_number_field
+            end
+
+            def set_default_number_of_approvals_required(number)
+              fill_element(:approvals_number_field, number)
+            end
+          end
+        end
+      end
+    end
+  end
+end

qa/qa/ee/page/project/settings/protected_branches.rb

@@ -12,6 +12,10 @@ module QA
               super
 
               base.class_eval do
+                view 'ee/app/views/projects/protected_branches/ee/_code_owner_approval_table.html.haml' do
+                  element :code_owner_toggle_button
+                end
+
                 view 'ee/app/views/projects/protected_branches/ee/_create_protected_branch.html.haml' do
                   element :allowed_to_push_select
                   element :allowed_to_push_dropdown
@@ -25,13 +29,18 @@ module QA
               end
             end
 
+            def require_code_owner_approval(branch)
+              toggle = find_element(:code_owner_toggle_button, branch_name: branch)
+              toggle.click unless toggle[:class].include?('is-checked')
+            end
+
             private
 
             def select_allowed(action, allowed)
               super
 
               # Click the select element again to close the dropdown
-              click_element :"allowed_to_#{action}_select"
+              click_element(:"allowed_to_#{action}_select")
             end
           end
         end

qa/qa/page/base.rb

@@ -99,7 +99,16 @@ module QA
       def find_element(name, **kwargs)
         wait_for_requests
 
-        find(element_selector_css(name), kwargs)
+        element_selector = element_selector_css(name, reject_capybara_query_keywords(kwargs))
+        find(element_selector, only_capybara_query_keywords(kwargs))
+      end
+
+      def only_capybara_query_keywords(kwargs)
+        kwargs.select { |kwarg| Capybara::Queries::SelectorQuery::VALID_KEYS.include?(kwarg) }
+      end
+
+      def reject_capybara_query_keywords(kwargs)
+        kwargs.reject { |kwarg| Capybara::Queries::SelectorQuery::VALID_KEYS.include?(kwarg) }
       end
 
       def active_element?(name)
@@ -162,11 +171,17 @@ module QA
       def has_element?(name, **kwargs)
         wait_for_requests
 
-        wait = kwargs.delete(:wait) || Capybara.default_max_wait_time
-        text = kwargs.delete(:text)
-        klass = kwargs.delete(:class)
+        disabled = kwargs.delete(:disabled)
 
-        has_css?(element_selector_css(name, kwargs), text: text, wait: wait, class: klass)
+        if disabled.nil?
+          wait = kwargs.delete(:wait) || Capybara.default_max_wait_time
+          text = kwargs.delete(:text)
+          klass = kwargs.delete(:class)
+
+          has_css?(element_selector_css(name, kwargs), text: text, wait: wait, class: klass)
+        else
+          find_element(name, kwargs).disabled? == disabled
+        end
       end
 
       def has_no_element?(name, **kwargs)

qa/qa/page/merge_request/show.rb

@@ -154,8 +154,8 @@ module QA
         end
 
         def merge!
-          click_element :merge_button if ready_to_merge?
-
+          wait_until_ready_to_merge
+          click_element(:merge_button)
           finished_loading?
 
           raise "Merge did not appear to be successful" unless merged?
@@ -165,11 +165,18 @@ module QA
           has_element?(:merged_status_content, text: 'The changes were merged into', wait: 30)
         end
 
-        def ready_to_merge?
-          # The merge button is disabled on load
-          wait_until do
-            has_element?(:merge_button)
-          end
+        # Check if the MR is able to be merged
+        # Waits up 10 seconds and returns false if the MR can't be merged
+        def mergeable?
+          # The merge button is enabled via JS, but `has_element?` calls
+          # `wait_for_requests`, which should ensure the disabled/enabled
+          # state of the element is reliable
+          has_element?(:merge_button, disabled: false)
+        end
+
+        # Waits up 60 seconds and raises an error if unable to merge
+        def wait_until_ready_to_merge
+          has_element?(:merge_button)
 
           # The merge button is enabled via JS
           wait_until(reload: false) do
@@ -198,7 +205,9 @@ module QA
         end
 
         def try_to_merge!
-          click_element :merge_button if ready_to_merge?
+          wait_until_ready_to_merge
+
+          click_element(:merge_button)
         end
 
         def view_email_patches

qa/qa/page/project/settings/advanced.rb

@@ -43,7 +43,9 @@ module QA
 
           def transfer_project!(project_name, namespace)
             expand_select_list
-            select_transfer_option(namespace)
+            # Workaround for a failure to search when there are no spaces around the /
+            # https://gitlab.com/gitlab-org/gitlab/-/issues/218965
+            select_transfer_option(namespace.gsub(/([^\s])\/([^\s])/, '\1 / \2'))
             click_element(:transfer_button)
             fill_confirmation_text(project_name)
             click_confirm_button

qa/qa/page/project/settings/main.rb

@@ -58,3 +58,5 @@ module QA
     end
   end
 end
+
+QA::Page::Project::Settings::Main.prepend_if_ee("QA::EE::Page::Project::Settings::Main")

qa/qa/resource/group.rb

@@ -14,6 +14,7 @@ module QA
         end
       end
 
+      attribute :full_path
       attribute :id
       attribute :name
       attribute :runners_token
@@ -74,10 +75,6 @@ module QA
       def api_delete_path
         "/groups/#{id}"
       end
-
-      def full_path
-        sandbox.path + ' / ' + path
-      end
     end
   end
 end

qa/qa/resource/project.rb

@@ -30,6 +30,8 @@ module QA
         "#{sandbox_path}#{group.path}/#{name}" if group
       end
 
+      alias_method :full_path, :path_with_namespace
+
       def sandbox_path
         group.respond_to?('sandbox') ? "#{group.sandbox.path}/" : ''
       end

qa/qa/resource/protected_branch.rb

@@ -5,7 +5,12 @@ require 'securerandom'
 module QA
   module Resource
     class ProtectedBranch < Base
-      attr_accessor :branch_name, :allowed_to_push, :allowed_to_merge, :protected
+      attr_accessor :branch_name,
+                    :allowed_to_push,
+                    :allowed_to_merge,
+                    :protected,
+                    :new_branch,
+                    :require_code_owner_approval
 
       attribute :project do
         Project.fabricate_via_api! do |resource|
@@ -21,11 +26,12 @@ module QA
           project_push.commit_message = 'Add new file'
           project_push.branch_name = branch_name
           project_push.new_branch = true
-          project_push.remote_branch = @branch_name
+          project_push.remote_branch = branch_name
         end
       end
 
       def initialize
+        @new_branch = true
         @branch_name = 'test/branch'
         @allowed_to_push = {
           roles: Resource::ProtectedBranch::Roles::DEVS_AND_MAINTAINERS
@@ -34,22 +40,29 @@ module QA
           roles: Resource::ProtectedBranch::Roles::DEVS_AND_MAINTAINERS
         }
         @protected = false
+        @require_code_owner_approval = true
       end
 
       def fabricate!
-        populate(:branch)
+        if new_branch
+          populate(:branch)
 
-        project.wait_for_push_new_branch @branch_name
+          project.wait_for_push_new_branch branch_name
+        end
 
         project.visit!
         Page::Project::Menu.perform(&:go_to_repository_settings)
 
         Page::Project::Settings::Repository.perform do |setting|
           setting.expand_protected_branches do |page|
-            page.select_branch(branch_name)
-            page.select_allowed_to_merge(allowed_to_merge)
-            page.select_allowed_to_push(allowed_to_push)
-            page.protect_branch
+            if new_branch
+              page.select_branch(branch_name)
+              page.select_allowed_to_merge(allowed_to_merge)
+              page.select_allowed_to_push(allowed_to_push)
+              page.protect_branch
+            else
+              page.require_code_owner_approval(branch_name) if require_code_owner_approval
+            end
           end
         end
       end
@@ -59,11 +72,11 @@ module QA
       end
 
       def api_get_path
-        "/projects/#{@project.api_resource[:id]}/protected_branches/#{@branch_name}"
+        "/projects/#{project.id}/protected_branches/#{branch_name}"
       end
 
       def api_delete_path
-        "/projects/#{@project.api_resource[:id]}/protected_branches/#{@branch_name}"
+        "/projects/#{project.id}/protected_branches/#{branch_name}"
       end
 
       class Roles

qa/qa/resource/sandbox.rb

@@ -13,6 +13,7 @@ module QA
 
       attribute :id
       attribute :runners_token
+      attribute :name
 
       def initialize
         @path = Runtime::Namespace.sandbox_name

qa/qa/runtime/feature.rb

@@ -28,19 +28,11 @@ module QA
       end
 
       def enable_and_verify(key)
-        Support::Retrier.retry_on_exception(sleep_interval: 2) do
-          enable(key)
-
-          is_enabled = false
-
-          QA::Support::Waiter.wait_until(sleep_interval: 1) do
-            is_enabled = enabled?(key)
-          end
-
-          raise SetFeatureError, "#{key} was not enabled!" unless is_enabled
+        set_and_verify(key, enable: true)
+      end
 
-          QA::Runtime::Logger.info("Successfully enabled and verified feature flag: #{key}")
-        end
+      def disable_and_verify(key)
+        set_and_verify(key, enable: false)
       end
 
       def enabled?(key)
@@ -75,6 +67,27 @@ module QA
         end
       end
 
+      # Change a feature flag and verify that the change was successful
+      # Arguments:
+      #   key: The feature flag to set (as a string)
+      #   enable: `true` to enable the flag, `false` to disable it
+      def set_and_verify(key, enable:)
+        Support::Retrier.retry_on_exception(sleep_interval: 2) do
+          enable ? enable(key) : disable(key)
+
+          is_enabled = nil
+
+          QA::Support::Waiter.wait_until(sleep_interval: 1) do
+            is_enabled = enabled?(key)
+            is_enabled == enable
+          end
+
+          raise SetFeatureError, "#{key} was not #{enable ? 'enabled' : 'disabled'}!" unless is_enabled == enable
+
+          QA::Runtime::Logger.info("Successfully #{enable ? 'enabled' : 'disabled'} and verified feature flag: #{key}")
+        end
+      end
+
       def set_feature(key, value)
         request = Runtime::API::Request.new(api_client, "/features/#{key}")
         response = post(request.url, { value: value })

qa/qa/specs/features/ee/browser_ui/3_create/repository/merge_with_code_owner_in_root_group_spec.rb

+# frozen_string_literal: true
+
+module QA
+  # These tests will fail unless the feature flag `skip_web_ui_code_owner_validations` is enabled.
+  # That requirement is temporary. See https://gitlab.com/gitlab-org/gitlab/-/issues/217427
+  # When the flag is no longer needed:
+  #  - the tests will no longer need to toggle it, and
+  #  - the tests will not require admin access, and
+  #  - the tests can be run in live environments
+  # Tracked in https://gitlab.com/gitlab-org/quality/team-tasks/-/issues/511
+  context 'Create', :requires_admin, :skip_live_env do
+    describe 'Codeowners' do
+      before(:context) do
+        @feature_flag = 'skip_web_ui_code_owner_validations'
+        @feature_flag_enabled = Runtime::Feature.enabled?(@feature_flag)
+        Runtime::Feature.enable_and_verify(@feature_flag) unless @feature_flag_enabled
+      end
+
+      after(:context) do
+        Runtime::Feature.disable_and_verify(@feature_flag) unless @feature_flag_enabled
+      end
+
+      context 'when the project is in the root group' do
+        let(:approver) { Resource::User.fabricate_or_use(Runtime::Env.gitlab_qa_username_1, Runtime::Env.gitlab_qa_password_1) }
+        let(:root_group) { Resource::Sandbox.fabricate_via_api! }
+        let(:project) do
+          Resource::Project.fabricate_via_api! do |project|
+            project.group = root_group
+            project.name = "code-owner-approve-and-merge"
+            project.initialize_with_readme = true
+          end
+        end
+
+        before do
+          group_or_project.add_member(approver, Resource::Members::AccessLevel::MAINTAINER)
+
+          Flow::Login.sign_in
+
+          project.visit!
+        end
+
+        after do
+          group_or_project.remove_member(approver)
+        end
+
+        context 'and the code owner is the root group' do
+          let(:codeowner) { root_group.path }
+          let(:group_or_project) { root_group }
+
+          it_behaves_like 'code owner merge request'
+        end
+
+        context 'and the code owner is a user' do
+          let(:codeowner) { approver.username }
+          let(:group_or_project) { project }
+
+          it_behaves_like 'code owner merge request'
+        end
+      end
+    end
+  end
+end

qa/qa/specs/features/ee/browser_ui/3_create/repository/merge_with_code_owner_in_subgroup_spec.rb

+# frozen_string_literal: true
+
+module QA
+  # These tests will fail unless the feature flag `skip_web_ui_code_owner_validations` is enabled.
+  # That requirement is temporary. See https://gitlab.com/gitlab-org/gitlab/-/issues/217427
+  # When the flag is no longer needed:
+  #  - the tests will no longer need to toggle it, and
+  #  - the tests will not require admin access, and
+  #  - the tests can be run in live environments
+  # Tracked in https://gitlab.com/gitlab-org/quality/team-tasks/-/issues/511
+  context 'Create', :requires_admin, :skip_live_env do
+    describe 'Codeowners' do
+      before(:context) do
+        @feature_flag = 'skip_web_ui_code_owner_validations'
+        @feature_flag_enabled = Runtime::Feature.enabled?(@feature_flag)
+        Runtime::Feature.enable_and_verify(@feature_flag) unless @feature_flag_enabled
+      end
+
+      after(:context) do
+        Runtime::Feature.disable_and_verify(@feature_flag) unless @feature_flag_enabled
+      end
+
+      context 'when the project is in a subgroup' do
+        let(:approver) { Resource::User.fabricate_or_use(Runtime::Env.gitlab_qa_username_1, Runtime::Env.gitlab_qa_password_1) }
+        let(:project) do
+          Resource::Project.fabricate_via_api! do |project|
+            project.name = "approve-and-merge"
+            project.initialize_with_readme = true
+          end
+        end
+
+        before do
+          group_or_project.add_member(approver, Resource::Members::AccessLevel::MAINTAINER)
+
+          Flow::Login.sign_in
+
+          project.visit!
+        end
+
+        after do
+          group_or_project.remove_member(approver)
+        end
+
+        context 'and the code owner is the root group' do
+          let(:codeowner) { project.group.sandbox.path }
+          let(:group_or_project) { project.group.sandbox }
+
+          it_behaves_like 'code owner merge request'
+        end
+
+        context 'and the code owner is the subgroup' do
+          let(:codeowner) { project.group.full_path }
+          let(:group_or_project) { project.group }
+
+          it_behaves_like 'code owner merge request'
+        end
+
+        context 'and the code owner is a user' do
+          let(:codeowner) { approver.username }
+          let(:group_or_project) { project }
+
+          it_behaves_like 'code owner merge request'
+        end
+      end
+    end
+  end
+end

qa/spec/runtime/feature_spec.rb

@@ -25,6 +25,21 @@ describe QA::Runtime::Feature do
     end
   end
 
+  describe '.enable_and_verify' do
+    it 'enables a feature flag' do
+      allow(described_class).to receive(:get).and_return(response_get)
+
+      expect(QA::Runtime::API::Request).to receive(:new)
+        .with(api_client, "/features/a-flag").and_return(request)
+      expect(described_class).to receive(:post)
+        .with(request.url, { value: true }).and_return(response_post)
+      expect(QA::Runtime::API::Request).to receive(:new)
+        .with(api_client, "/features").and_return(request)
+
+      subject.enable_and_verify('a-flag')
+    end
+  end
+
   describe '.disable' do
     it 'disables a feature flag' do
       expect(QA::Runtime::API::Request)
@@ -40,6 +55,22 @@ describe QA::Runtime::Feature do
     end
   end
 
+  describe '.disable_and_verify' do
+    it 'disables a feature flag' do
+      allow(described_class).to receive(:get)
+        .and_return(Struct.new(:code, :body).new(200, '[{ "name": "a-flag", "state": "off" }]'))
+
+      expect(QA::Runtime::API::Request).to receive(:new)
+        .with(api_client, "/features/a-flag").and_return(request)
+      expect(described_class).to receive(:post)
+        .with(request.url, { value: false }).and_return(response_post)
+      expect(QA::Runtime::API::Request).to receive(:new)
+        .with(api_client, "/features").and_return(request)
+
+      subject.disable_and_verify('a-flag')
+    end
+  end
+
   describe '.enabled?' do
     it 'returns a feature flag state' do
       expect(QA::Runtime::API::Request)

qa/spec/support/shared_examples/merge_with_code_owner_shared_examples.rb

+# frozen_string_literal: true
+
+module QA
+  shared_examples 'code owner merge request' do
+    let(:branch_name) { 'new-branch' }
+
+    it 'is approved and merged' do
+      # Require one approval from any eligible user on any branch
+      # This will confirm that this type of unrestricted approval is
+      # also satisfied when a code owner grants approval
+      Page::Project::Menu.perform(&:go_to_general_settings)
+      Page::Project::Settings::Main.perform do |main|
+        main.expand_merge_request_approvals_settings do |settings|
+          settings.set_default_number_of_approvals_required(1)
+        end
+      end
+
+      Resource::Repository::Commit.fabricate_via_api! do |commit|
+        commit.project = project
+        commit.commit_message = 'Add CODEOWNERS'
+        commit.add_files(
+          [
+            {
+              file_path: 'CODEOWNERS',
+              content: <<~CONTENT
+                README.md @#{codeowner}
+              CONTENT
+            }
+          ]
+        )
+      end
+
+      # Require approval from code owners on master
+      Resource::ProtectedBranch.fabricate! do |protected_branch|
+        protected_branch.project = project
+        protected_branch.branch_name = 'master'
+        protected_branch.new_branch = false
+        protected_branch.require_code_owner_approval = true
+      end
+
+      # Push a change to the file with a CODEOWNERS rule
+      Resource::Repository::Push.fabricate! do |push|
+        push.repository_http_uri = project.repository_http_location.uri
+        push.branch_name = branch_name
+        push.file_name = 'README.md'
+        push.file_content = 'Updated'
+      end
+
+      merge_request = Resource::MergeRequest.fabricate! do |merge_request|
+        merge_request.project = project
+        merge_request.target_new_branch = false
+        merge_request.source_branch = branch_name
+        merge_request.no_preparation = true
+      end
+
+      Flow::Login.while_signed_in(as: approver) do
+        merge_request.visit!
+
+        Page::MergeRequest::Show.perform do |merge_request|
+          expect(merge_request.approvals_required_from).to include('Code Owners')
+          expect(merge_request).not_to be_mergeable
+
+          merge_request.click_approve
+          merge_request.merge!
+
+          expect(merge_request).to be_merged
+        end
+      end
+    end
+  end
+end