From e5a69f5e2c2f9c84b0b00ac6a004203dc3fe24d9 Mon Sep 17 00:00:00 2001 From: Joe Woodward Date: Fri, 14 Feb 2025 12:25:02 +0000 Subject: [PATCH] Update merge request widget serializers This change fetches the squash options from the merge request instead of the project which now includes custom options from protected branches. This also removes the presentation from these expose calls as it's not required. I've moved one of the EE specs under the EE namespace as this class is extended in EE Related to https://gitlab.com/gitlab-org/gitlab/-/issues/503235 --- .../rspec/factory_bot/avoid_create.yml | 2 +- .rubocop_todo/rspec/verified_doubles.yml | 2 +- ...merge_request_poll_cached_widget_entity.rb | 12 +- .../merge_request_poll_widget_entity.rb | 12 +- ..._request_poll_cached_widget_entity_spec.rb | 138 ++++++++++++--- .../merge_request_poll_widget_entity_spec.rb | 162 ++++++++++++++++++ .../merge_request_poll_widget_entity_spec.rb | 61 ------- .../projects/branch_rules/squash_options.rb | 3 +- .../merge_request_poll_widget_entity_spec.rb | 2 +- 9 files changed, 290 insertions(+), 104 deletions(-) create mode 100644 ee/spec/serializers/ee/merge_request_poll_widget_entity_spec.rb delete mode 100644 ee/spec/serializers/merge_request_poll_widget_entity_spec.rb diff --git a/.rubocop_todo/rspec/factory_bot/avoid_create.yml b/.rubocop_todo/rspec/factory_bot/avoid_create.yml index f04a6053301ee3..36eaf7c51a7903 100644 --- a/.rubocop_todo/rspec/factory_bot/avoid_create.yml +++ b/.rubocop_todo/rspec/factory_bot/avoid_create.yml @@ -133,6 +133,7 @@ RSpec/FactoryBot/AvoidCreate: - 'ee/spec/serializers/ee/issue_sidebar_basic_entity_spec.rb' - 'ee/spec/serializers/ee/issue_sidebar_extras_entity_spec.rb' - 'ee/spec/serializers/ee/merge_request_poll_cached_widget_entity_spec.rb' + - 'ee/spec/serializers/ee/merge_request_poll_widget_entity_spec.rb' - 'ee/spec/serializers/ee/note_entity_spec.rb' - 'ee/spec/serializers/ee/user_serializer_spec.rb' - 'ee/spec/serializers/environment_entity_spec.rb' @@ -154,7 +155,6 @@ RSpec/FactoryBot/AvoidCreate: - 'ee/spec/serializers/linked_feature_flag_issue_entity_spec.rb' - 'ee/spec/serializers/member_entity_spec.rb' - 'ee/spec/serializers/member_user_entity_spec.rb' - - 'ee/spec/serializers/merge_request_poll_widget_entity_spec.rb' - 'ee/spec/serializers/merge_request_widget_entity_spec.rb' - 'ee/spec/serializers/pipeline_serializer_spec.rb' - 'ee/spec/serializers/productivity_analytics_merge_request_entity_spec.rb' diff --git a/.rubocop_todo/rspec/verified_doubles.yml b/.rubocop_todo/rspec/verified_doubles.yml index 1f5cb1f96e291a..b955a71906bff4 100644 --- a/.rubocop_todo/rspec/verified_doubles.yml +++ b/.rubocop_todo/rspec/verified_doubles.yml @@ -96,6 +96,7 @@ RSpec/VerifiedDoubles: - 'ee/spec/serializers/ee/issue_entity_spec.rb' - 'ee/spec/serializers/ee/issue_sidebar_extras_entity_spec.rb' - 'ee/spec/serializers/ee/merge_request_poll_cached_widget_entity_spec.rb' + - 'ee/spec/serializers/ee/merge_request_poll_widget_entity_spec.rb' - 'ee/spec/serializers/ee/note_entity_spec.rb' - 'ee/spec/serializers/environment_entity_spec.rb' - 'ee/spec/serializers/epic_entity_spec.rb' @@ -104,7 +105,6 @@ RSpec/VerifiedDoubles: - 'ee/spec/serializers/integrations/jira_serializers/issue_entity_spec.rb' - 'ee/spec/serializers/issues/linked_issue_feature_flag_entity_spec.rb' - 'ee/spec/serializers/linked_feature_flag_issue_entity_spec.rb' - - 'ee/spec/serializers/merge_request_poll_widget_entity_spec.rb' - 'ee/spec/serializers/merge_request_widget_entity_spec.rb' - 'ee/spec/serializers/test_reports_comparer_serializer_spec.rb' - 'ee/spec/serializers/user_analytics_entity_spec.rb' diff --git a/app/serializers/merge_request_poll_cached_widget_entity.rb b/app/serializers/merge_request_poll_cached_widget_entity.rb index a7e7aafd07b17a..df119d8516b795 100644 --- a/app/serializers/merge_request_poll_cached_widget_entity.rb +++ b/app/serializers/merge_request_poll_cached_widget_entity.rb @@ -89,17 +89,11 @@ class MergeRequestPollCachedWidgetEntity < IssuableEntity diffs_project_merge_request_path(merge_request.project, merge_request) end - expose :squash_enabled_by_default do |merge_request| - presenter(merge_request).project.squash_enabled_by_default? - end + expose :squash_enabled_by_default?, as: :squash_enabled_by_default - expose :squash_readonly do |merge_request| - presenter(merge_request).project.squash_readonly? - end + expose :squash_readonly?, as: :squash_readonly - expose :squash_on_merge do |merge_request| - presenter(merge_request).squash_on_merge? - end + expose :squash_on_merge?, as: :squash_on_merge expose :api_approvals_path do |merge_request| presenter(merge_request).api_approvals_path diff --git a/app/serializers/merge_request_poll_widget_entity.rb b/app/serializers/merge_request_poll_widget_entity.rb index 3ebbd2873f3a03..3617713d03d70b 100644 --- a/app/serializers/merge_request_poll_widget_entity.rb +++ b/app/serializers/merge_request_poll_widget_entity.rb @@ -108,17 +108,11 @@ class MergeRequestPollWidgetEntity < Grape::Entity presenter(merge_request).revert_in_fork_path end - expose :squash_enabled_by_default do |merge_request| - presenter(merge_request).project.squash_enabled_by_default? - end + expose :squash_enabled_by_default?, as: :squash_enabled_by_default - expose :squash_readonly do |merge_request| - presenter(merge_request).project.squash_readonly? - end + expose :squash_readonly?, as: :squash_readonly - expose :squash_on_merge do |merge_request| - presenter(merge_request).squash_on_merge? - end + expose :squash_on_merge?, as: :squash_on_merge expose :approvals_widget_type do |merge_request| presenter(merge_request).approvals_widget_type diff --git a/ee/spec/serializers/ee/merge_request_poll_cached_widget_entity_spec.rb b/ee/spec/serializers/ee/merge_request_poll_cached_widget_entity_spec.rb index c8a49d2fb4f173..f382222b161f5f 100644 --- a/ee/spec/serializers/ee/merge_request_poll_cached_widget_entity_spec.rb +++ b/ee/spec/serializers/ee/merge_request_poll_cached_widget_entity_spec.rb @@ -2,20 +2,34 @@ require 'spec_helper' -RSpec.describe MergeRequestPollCachedWidgetEntity do - let_it_be(:project) { create(:project, :repository) } - let_it_be_with_reload(:resource) { create(:merge_request, source_project: project, target_project: project) } +RSpec.describe MergeRequestPollCachedWidgetEntity, feature_category: :merge_trains do + using RSpec::Parameterized::TableSyntax + + let_it_be_with_refind(:project) { create(:project, :repository) } let_it_be(:user) { create(:user) } + let_it_be(:target_branch) { 'feature' } let(:request) { double('request', current_user: user, project: project) } + let(:title) { 'MR title' } + let(:description) { 'MR description' } + let(:merge_request) do + create( + :merge_request, + source_project: project, + target_project: project, + target_branch: target_branch, + title: title, + description: description + ) + end - subject { described_class.new(resource, request: request).as_json } + subject(:entity) { described_class.new(merge_request, request: request).as_json } it 'includes policy violation status' do is_expected.to include(:policy_violation) end - context 'jira_associations' do + describe 'jira_associations' do context 'when feature is available' do let_it_be(:jira_integration) { create(:jira_integration, project: project, active: true) } @@ -28,31 +42,24 @@ shared_examples 'contains the issue key specified in MR title / description' do context 'when Jira issue is provided in MR title' do let(:issue_key) { 'SIGNUP-1234' } + let(:title) { "Fixes sign up issue #{issue_key}" } - before do - resource.update!(title: "Fixes sign up issue #{issue_key}") - end - - it { expect(subject[:jira_associations][:issue_keys]).to contain_exactly(issue_key) } + it { expect(entity[:jira_associations][:issue_keys]).to contain_exactly(issue_key) } end context 'when Jira issue is provided in MR description' do let(:issue_key) { 'SECURITY-1234' } + let(:description) { "Related to #{issue_key}" } - before do - resource.update!(description: "Related to #{issue_key}") - end - - it { expect(subject[:jira_associations][:issue_keys]).to contain_exactly(issue_key) } + it { expect(entity[:jira_associations][:issue_keys]).to contain_exactly(issue_key) } end end shared_examples 'when issue key is NOT specified in MR title / description' do - before do - resource.update!(title: "Fixes sign up issue", description: "Prevent spam sign ups by adding a rate limiter") - end + let(:title) { "Fixes sign up issue" } + let(:description) { "Prevent spam sign ups by adding a rate limiter" } - it { expect(subject[:jira_associations][:issue_keys]).to be_empty } + it { expect(entity[:jira_associations][:issue_keys]).to be_empty } end context 'when jira issue is required for merge' do @@ -60,7 +67,7 @@ project.create_project_setting(prevent_merge_without_jira_issue: true) end - it { expect(subject[:jira_associations][:enforced]).to be_truthy } + it { expect(entity[:jira_associations][:enforced]).to be_truthy } it_behaves_like 'contains the issue key specified in MR title / description' it_behaves_like 'when issue key is NOT specified in MR title / description' @@ -71,7 +78,7 @@ project.create_project_setting(prevent_merge_without_jira_issue: false) end - it { expect(subject[:jira_associations][:enforced]).to be_falsey } + it { expect(entity[:jira_associations][:enforced]).to be_falsey } it_behaves_like 'contains the issue key specified in MR title / description' it_behaves_like 'when issue key is NOT specified in MR title / description' @@ -92,4 +99,93 @@ end end end + + describe 'squash fields' do + context 'when branch_rule_squash_settings feature is enabled' do + before do + stub_feature_flags(branch_rule_squash_settings: true) + end + + context 'when branch rule squash option is defined for target branch' do + let_it_be(:protected_branch) { create(:protected_branch, name: target_branch, project: project) } + let_it_be(:branch_rule_squash_option) do + create(:branch_rule_squash_option, project: project, protected_branch: protected_branch) + end + + where(:project_squash_option, :squash_option, :value, :default, :readonly) do + 'default_off' | 'always' | true | true | true + 'default_on' | 'never' | false | false | true + 'never' | 'default_on' | false | true | false + 'always' | 'default_off' | false | false | false + end + + with_them do + before do + project.project_setting.update!(squash_option: project_squash_option) + branch_rule_squash_option.update!(squash_option: squash_option) + end + + it 'the key reflects the project squash option value' do + expect(entity[:squash_on_merge]).to eq(value) + expect(entity[:squash_enabled_by_default]).to eq(default) + expect(entity[:squash_readonly]).to eq(readonly) + end + end + end + + context 'when no branch rule squash option exists' do + where(:project_squash_option, :value, :default, :readonly) do + 'always' | true | true | true + 'never' | false | false | true + 'default_on' | false | true | false + 'default_off' | false | false | false + end + + with_them do + before do + project.project_setting.update!(squash_option: project_squash_option) + end + + it 'the key reflects the project squash option value' do + expect(entity[:squash_on_merge]).to eq(value) + expect(entity[:squash_enabled_by_default]).to eq(default) + expect(entity[:squash_readonly]).to eq(readonly) + end + end + end + end + + context 'when branch_rule_squash_settings feature is disabled' do + before do + stub_feature_flags(branch_rule_squash_settings: false) + end + + describe 'squash defaults for projects' do + let_it_be(:protected_branch) { create(:protected_branch, name: target_branch, project: project) } + let_it_be(:branch_rule_squash_option) do + create(:branch_rule_squash_option, project: project, protected_branch: protected_branch) + end + + where(:project_squash_option, :squash_option, :value, :default, :readonly) do + 'always' | 'default_off' | true | true | true + 'never' | 'default_on' | false | false | true + 'default_on' | 'never' | false | true | false + 'default_off' | 'always' | false | false | false + end + + with_them do + before do + project.project_setting.update!(squash_option: project_squash_option) + branch_rule_squash_option.update!(squash_option: squash_option) + end + + it 'the key reflects the project squash option value' do + expect(entity[:squash_on_merge]).to eq(value) + expect(entity[:squash_enabled_by_default]).to eq(default) + expect(entity[:squash_readonly]).to eq(readonly) + end + end + end + end + end end diff --git a/ee/spec/serializers/ee/merge_request_poll_widget_entity_spec.rb b/ee/spec/serializers/ee/merge_request_poll_widget_entity_spec.rb new file mode 100644 index 00000000000000..2a7a9e04cc2f23 --- /dev/null +++ b/ee/spec/serializers/ee/merge_request_poll_widget_entity_spec.rb @@ -0,0 +1,162 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequestPollWidgetEntity, feature_category: :merge_trains do + include ProjectForksHelper + using RSpec::Parameterized::TableSyntax + + let_it_be(:user) { create(:user) } + let_it_be(:target_branch) { 'feature' } + + let(:project) { create(:project, :repository, developers: user) } + let(:request) { double('request', current_user: user) } + let(:merge_request) do + create(:merge_request, source_project: project, target_project: project, target_branch: target_branch) + end + + subject(:entity) do + described_class.new(merge_request, current_user: user, request: request).as_json + end + + describe 'Merge Trains' do + let!(:merge_train) { create(:merge_train_car, merge_request: merge_request) } + + before do + stub_licensed_features(merge_pipelines: true, merge_trains: true) + project.update!(merge_pipelines_enabled: true, merge_trains_enabled: true) + end + + it 'has merge train entity' do + expect(entity).to include(:merge_trains_skip_train_allowed) + end + end + + describe 'auto merge' do + context 'when head pipeline is running' do + before do + create( + :ci_pipeline, :running, + project: project, + ref: merge_request.source_branch, + sha: merge_request.diff_head_sha + ) + merge_request.update_head_pipeline + end + + it 'returns available auto merge strategies' do + expect(entity[:available_auto_merge_strategies]).to( + eq(%w[merge_when_checks_pass]) + ) + end + end + + context 'when head pipeline is finished and approvals are pending' do + before do + create(:approval_merge_request_rule, merge_request: merge_request, approvals_required: 1, users: [user]) + create( + :ci_pipeline, :success, + project: project, + ref: merge_request.source_branch, + sha: merge_request.diff_head_sha + ) + merge_request.update_head_pipeline + end + + it 'returns available auto merge strategies' do + expect(entity[:available_auto_merge_strategies]).to( + eq(%w[merge_when_checks_pass]) + ) + end + end + end + + describe 'squash fields' do + context 'when branch_rule_squash_settings feature is enabled' do + before do + stub_feature_flags(branch_rule_squash_settings: true) + end + + context 'when branch rule squash option is defined for target branch' do + let(:protected_branch) { create(:protected_branch, name: target_branch, project: project) } + let(:branch_rule_squash_option) do + create(:branch_rule_squash_option, project: project, protected_branch: protected_branch) + end + + where(:project_squash_option, :squash_option, :value, :default, :readonly) do + 'default_off' | 'always' | true | true | true + 'default_on' | 'never' | false | false | true + 'never' | 'default_on' | false | true | false + 'always' | 'default_off' | false | false | false + end + + with_them do + before do + project.project_setting.update!(squash_option: project_squash_option) + branch_rule_squash_option.update!(squash_option: squash_option) + end + + it 'the key reflects the project squash option value' do + expect(entity[:squash_on_merge]).to eq(value) + expect(entity[:squash_enabled_by_default]).to eq(default) + expect(entity[:squash_readonly]).to eq(readonly) + end + end + end + + context 'when no branch rule squash option exists' do + where(:project_squash_option, :value, :default, :readonly) do + 'always' | true | true | true + 'never' | false | false | true + 'default_on' | false | true | false + 'default_off' | false | false | false + end + + with_them do + before do + project.project_setting.update!(squash_option: project_squash_option) + end + + it 'the key reflects the project squash option value' do + expect(entity[:squash_on_merge]).to eq(value) + expect(entity[:squash_enabled_by_default]).to eq(default) + expect(entity[:squash_readonly]).to eq(readonly) + end + end + end + end + + context 'when branch_rule_squash_settings feature is disabled' do + before do + stub_feature_flags(branch_rule_squash_settings: false) + end + + describe 'squash defaults for projects' do + let(:protected_branch) { create(:protected_branch, name: target_branch, project: project) } + let(:branch_rule_squash_option) do + create(:branch_rule_squash_option, project: project, protected_branch: protected_branch) + end + + where(:project_squash_option, :squash_option, :value, :default, :readonly) do + 'always' | 'default_off' | true | true | true + 'never' | 'default_on' | false | false | true + 'default_on' | 'never' | false | true | false + 'default_off' | 'always' | false | false | false + end + + with_them do + before do + project.project_setting.update!(squash_option: project_squash_option) + branch_rule_squash_option.update!(squash_option: squash_option) + end + + it 'the key reflects the project squash option value' do + expect(entity[:squash_on_merge]).to eq(value) + expect(entity[:squash_enabled_by_default]).to eq(default) + expect(entity[:squash_readonly]).to eq(readonly) + end + end + end + end + end +end diff --git a/ee/spec/serializers/merge_request_poll_widget_entity_spec.rb b/ee/spec/serializers/merge_request_poll_widget_entity_spec.rb deleted file mode 100644 index fbc141c7a6021b..00000000000000 --- a/ee/spec/serializers/merge_request_poll_widget_entity_spec.rb +++ /dev/null @@ -1,61 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe MergeRequestPollWidgetEntity, feature_category: :merge_trains do - include ProjectForksHelper - - let_it_be(:user) { create(:user) } - let_it_be(:project) { create(:project, :repository, developers: user) } - let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } - - let(:request) { double('request', current_user: user) } - - subject(:entity) do - described_class.new(merge_request, current_user: user, request: request).as_json - end - - describe 'Merge Trains' do - let!(:merge_train) { create(:merge_train_car, merge_request: merge_request) } - - before do - stub_licensed_features(merge_pipelines: true, merge_trains: true) - project.update!(merge_pipelines_enabled: true, merge_trains_enabled: true) - end - - it 'has merge train entity' do - expect(entity).to include(:merge_trains_skip_train_allowed) - end - end - - describe 'auto merge' do - context 'when head pipeline is running' do - before do - create(:ci_pipeline, :running, - project: project, ref: merge_request.source_branch, sha: merge_request.diff_head_sha) - merge_request.update_head_pipeline - end - - it 'returns available auto merge strategies' do - expect(entity[:available_auto_merge_strategies]).to( - eq(%w[merge_when_checks_pass]) - ) - end - end - - context 'when head pipeline is finished and approvals are pending' do - before do - create(:approval_merge_request_rule, merge_request: merge_request, approvals_required: 1, users: [user]) - create(:ci_pipeline, :success, - project: project, ref: merge_request.source_branch, sha: merge_request.diff_head_sha) - merge_request.update_head_pipeline - end - - it 'returns available auto merge strategies' do - expect(entity[:available_auto_merge_strategies]).to( - eq(%w[merge_when_checks_pass]) - ) - end - end - end -end diff --git a/spec/factories/projects/branch_rules/squash_options.rb b/spec/factories/projects/branch_rules/squash_options.rb index 750afb69a39be6..2ad36578567cca 100644 --- a/spec/factories/projects/branch_rules/squash_options.rb +++ b/spec/factories/projects/branch_rules/squash_options.rb @@ -2,7 +2,8 @@ FactoryBot.define do factory :branch_rule_squash_option, class: 'Projects::BranchRules::SquashOption' do - protected_branch + project + protected_branch { association :protected_branch, project: project } trait :always do squash_option { :always } diff --git a/spec/serializers/merge_request_poll_widget_entity_spec.rb b/spec/serializers/merge_request_poll_widget_entity_spec.rb index 2d2b9282173aa3..7101f465de4a71 100644 --- a/spec/serializers/merge_request_poll_widget_entity_spec.rb +++ b/spec/serializers/merge_request_poll_widget_entity_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe MergeRequestPollWidgetEntity do +RSpec.describe MergeRequestPollWidgetEntity, feature_category: :merge_trains do include ProjectForksHelper using RSpec::Parameterized::TableSyntax -- GitLab