From 414b947d3192ff80772eb001d2c1480fcf35e248 Mon Sep 17 00:00:00 2001 From: Jessie Young Date: Wed, 7 Dec 2022 10:10:37 -0800 Subject: [PATCH 1/4] Add ability to add read_code to custom roles * download_code and read_code are now separate via https://gitlab.com/gitlab-org/gitlab/-/issues/376180 * for the first iteration of a customer-facing MVC, we want to provide the ability to customize `read_code` for a Guest user. * We previously added the ability to customize `download_code` but we will be removing that for the customer MVC. * This is all behind the `customizable_roles` feature flag * https://gitlab.com/gitlab-org/gitlab/-/issues/20277 Changelog: added --- app/models/members/member_role.rb | 3 + ...206222032_add_read_code_to_member_roles.rb | 7 ++ db/schema_migrations/20221206222032 | 1 + db/structure.sql | 3 +- ee/app/models/ee/user.rb | 4 +- ...user_member_roles_in_projects_preloader.rb | 16 +-- ee/app/policies/ee/project_policy.rb | 8 +- ee/lib/api/member_roles.rb | 23 ++-- ee/lib/ee/api/entities/member_role.rb | 2 +- .../lib/ee/api/entities/member_role_spec.rb | 4 +- ee/spec/models/ee/user_spec.rb | 28 ++--- ...member_roles_in_projects_preloader_spec.rb | 119 ++++++++---------- ee/spec/policies/project_policy_spec.rb | 46 +++---- ee/spec/requests/api/member_roles_spec.rb | 60 +++++---- 14 files changed, 167 insertions(+), 157 deletions(-) create mode 100644 db/migrate/20221206222032_add_read_code_to_member_roles.rb create mode 100644 db/schema_migrations/20221206222032 diff --git a/app/models/members/member_role.rb b/app/models/members/member_role.rb index 0661082dcedc48..e9d7b1d3f800a0 100644 --- a/app/models/members/member_role.rb +++ b/app/models/members/member_role.rb @@ -1,6 +1,9 @@ # frozen_string_literal: true class MemberRole < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass + include IgnorableColumns + ignore_column :download_code, remove_with: '15.9', remove_after: '2023-01-22' + has_many :members belongs_to :namespace diff --git a/db/migrate/20221206222032_add_read_code_to_member_roles.rb b/db/migrate/20221206222032_add_read_code_to_member_roles.rb new file mode 100644 index 00000000000000..dc62672ccd0c22 --- /dev/null +++ b/db/migrate/20221206222032_add_read_code_to_member_roles.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddReadCodeToMemberRoles < Gitlab::Database::Migration[2.1] + def change + add_column :member_roles, :read_code, :boolean, default: false + end +end diff --git a/db/schema_migrations/20221206222032 b/db/schema_migrations/20221206222032 new file mode 100644 index 00000000000000..16c8b6ea72b269 --- /dev/null +++ b/db/schema_migrations/20221206222032 @@ -0,0 +1 @@ +9e3f3c09100e3c26de7280bf30dc836a66d9fefb0894c86c80a3c5ee8e36235b \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index f7523d18f6b87e..abddbc5f8ad636 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -17357,7 +17357,8 @@ CREATE TABLE member_roles ( created_at timestamp with time zone NOT NULL, updated_at timestamp with time zone NOT NULL, base_access_level integer NOT NULL, - download_code boolean DEFAULT false + download_code boolean DEFAULT false, + read_code boolean DEFAULT false ); CREATE SEQUENCE member_roles_id_seq diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index 41fecc900c5880..daa7bc696031d3 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -474,9 +474,9 @@ def namespace_ban_for(namespace) namespace_bans.find_by!(namespace: namespace) end - def download_code_for?(project) + def read_code_for?(project) roles = preloaded_member_roles_for_projects([project])[project.id] - roles&.include?(:download_code) + roles&.include?(:read_code) end override :preloaded_member_roles_for_projects diff --git a/ee/app/models/preloaders/user_member_roles_in_projects_preloader.rb b/ee/app/models/preloaders/user_member_roles_in_projects_preloader.rb index 36efadb367f324..9c5b822ccdf12e 100644 --- a/ee/app/models/preloaders/user_member_roles_in_projects_preloader.rb +++ b/ee/app/models/preloaders/user_member_roles_in_projects_preloader.rb @@ -26,29 +26,29 @@ def execute value_list = Arel::Nodes::ValuesList.new(sql_values_array) sql = <<~SQL - SELECT project_ids.project_id, download_code_permissions.download_code FROM (#{value_list.to_sql}) AS project_ids (project_id, namespace_ids), + SELECT project_ids.project_id, custom_permissions.read_code FROM (#{value_list.to_sql}) AS project_ids (project_id, namespace_ids), LATERAL ( ( - #{Member.select('download_code') + #{Member.select('read_code') .left_outer_joins(:member_role) .where("members.source_type = 'Project' AND members.source_id = project_ids.project_id") .with_user(user) - .where(member_roles: { download_code: true }) + .where(member_roles: { read_code: true }) .limit(1).to_sql} ) UNION ALL ( - #{Member.select('download_code') + #{Member.select('read_code') .left_outer_joins(:member_role) .where("members.source_type = 'Namespace' AND members.source_id IN (SELECT UNNEST(project_ids.namespace_ids) as ids)") .with_user(user) - .where(member_roles: { download_code: true }) + .where(member_roles: { read_code: true }) .limit(1).to_sql} ) UNION ALL ( - SELECT false AS download_code + SELECT false AS read_code ) LIMIT 1 - ) AS download_code_permissions + ) AS custom_permissions SQL grouped_by_project = ApplicationRecord.connection.execute(sql).to_a.group_by do |h| @@ -57,7 +57,7 @@ def execute grouped_by_project.transform_values do |value| custom_attributes = [] - custom_attributes << :download_code if value.find { |custom_role| custom_role["download_code"] == true } + custom_attributes << :read_code if value.find { |custom_role| custom_role["read_code"] == true } custom_attributes end end diff --git a/ee/app/policies/ee/project_policy.rb b/ee/app/policies/ee/project_policy.rb index 86d24c63ee31eb..1d13c995e6a3b2 100644 --- a/ee/app/policies/ee/project_policy.rb +++ b/ee/app/policies/ee/project_policy.rb @@ -177,11 +177,11 @@ module ProjectPolicy ::Feature.enabled?(:customizable_roles, @subject.root_ancestor) end - desc "Custom role on project that enables download code" - condition(:role_enables_download_code) do + desc "Custom role on project that enables read code" + condition(:role_enables_read_code) do next unless @user.is_a?(User) - @user.download_code_for?(project) + @user.read_code_for?(project) end condition(:okrs_enabled, scope: :subject) { project&.okrs_mvc_feature_flag_enabled? } @@ -538,7 +538,7 @@ module ProjectPolicy enable :admin_merge_request_approval_settings end - rule { custom_roles_allowed & role_enables_download_code }.enable :download_code + rule { custom_roles_allowed & role_enables_read_code }.enable :read_code rule { can?(:create_issue) & okrs_enabled }.enable :create_objective diff --git a/ee/lib/api/member_roles.rb b/ee/lib/api/member_roles.rb index 29a4a9ebb1b484..2563d082721e9c 100644 --- a/ee/lib/api/member_roles.rb +++ b/ee/lib/api/member_roles.rb @@ -16,22 +16,25 @@ class MemberRoles < ::API::Base desc 'Get Member Roles for a group' do success EE::API::Entities::MemberRole end + get ":id/member_roles" do group = find_group(params[:id]) - member_roles = group.member_roles - present member_roles, with: EE::API::Entities::MemberRole end desc 'Create Member Role for a group' do success EE::API::Entities::MemberRole end + params do - requires 'base_access_level', type: Integer, values: Gitlab::Access.all_values, - desc: 'Base Access Level for the configured role ' - optional 'download_code', type: Boolean, - desc: 'Permission to download code' + requires( + 'base_access_level', + type: Integer, + values: Gitlab::Access.all_values, + desc: 'Base Access Level for the configured role' + ) + optional 'read_code', type: Boolean, desc: 'Permission to read code' end post ":id/member_roles" do @@ -49,9 +52,13 @@ class MemberRoles < ::API::Base desc 'Delete Member Role for a group' do success EE::API::Entities::MemberRole end + params do - requires 'member_role_id', type: Integer, - desc: 'The ID of the Member Role to be deleted' + requires( + 'member_role_id', + type: Integer, + desc: 'The ID of the Member Role to be deleted' + ) end delete ":id/member_roles/:member_role_id" do diff --git a/ee/lib/ee/api/entities/member_role.rb b/ee/lib/ee/api/entities/member_role.rb index a32121ab0f05c1..b98a75c99de4d4 100644 --- a/ee/lib/ee/api/entities/member_role.rb +++ b/ee/lib/ee/api/entities/member_role.rb @@ -7,7 +7,7 @@ class MemberRole < Grape::Entity expose :id expose :namespace_id, as: :group_id expose :base_access_level - expose :download_code + expose :read_code end end end diff --git a/ee/spec/lib/ee/api/entities/member_role_spec.rb b/ee/spec/lib/ee/api/entities/member_role_spec.rb index 4b96d657bf3001..4aa4e23f166390 100644 --- a/ee/spec/lib/ee/api/entities/member_role_spec.rb +++ b/ee/spec/lib/ee/api/entities/member_role_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe EE::API::Entities::MemberRole do - describe 'exposes access_level and download_code fields' do + describe 'exposes expected fields' do let_it_be(:group) { create(:group) } let_it_be(:user) { create(:user) } @@ -17,7 +17,7 @@ expect(subject[:id]).to eq member_role.id expect(subject[:base_access_level]).to eq member_role.base_access_level - expect(subject[:download_code]).to eq member_role.download_code + expect(subject[:read_code]).to eq member_role.read_code expect(subject[:group_id]).to eq(member_role.namespace.id) end end diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index 1f53eccf7b40e5..f5a708332dc10a 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -1946,7 +1946,7 @@ end end - describe '#download_code_for?', :request_store do + describe '#read_code_for?', :request_store do let_it_be(:project) { create(:project, :in_group) } let_it_be(:user) { create(:user) } @@ -1955,29 +1955,29 @@ create( :member_role, :guest, - download_code: true, + read_code: true, members: [project_member], namespace: project.group ) end - context 'when download_code present in preloaded custom roles' do + context 'when read_code present in preloaded custom roles' do before do - user.download_code_for?(project) + user.read_code_for?(project) end it 'returns true' do - expect(user.download_code_for?(project)).to be true + expect(user.read_code_for?(project)).to be true end it 'does not perform extra queries when asked for projects have already been preloaded' do - expect { user.download_code_for?(project) }.not_to exceed_query_limit(0) + expect { user.read_code_for?(project) }.not_to exceed_query_limit(0) end end context 'when project not present in preloaded custom roles' do it 'loads the custom role' do - expect(user.download_code_for?(project)).to be true + expect(user.read_code_for?(project)).to be true end end end @@ -1986,21 +1986,21 @@ let_it_be(:project) { create(:project, :private, :in_group) } let_it_be(:user) { create(:user) } let_it_be(:project_member) { create(:project_member, :guest, user: user, source: project) } - let_it_be(:member_role) { create(:member_role, :guest, download_code: true, members: [project_member], namespace: project.group) } + let_it_be(:member_role) { create(:member_role, :guest, read_code: true, members: [project_member], namespace: project.group) } context 'when custom roles are present' do - context 'when custom role enables download code' do - it 'returns hash with project ids as keys and download_code in value' do + context 'when custom role enables read code' do + it 'returns hash with project ids as keys and read_code in value' do preloaded = user.preloaded_member_roles_for_projects([project]) - expect(preloaded).to eq({ project.id => [:download_code] }) + expect(preloaded).to eq({ project.id => [:read_code] }) end end - context 'when custom role does not enable download code' do + context 'when custom role does not enable read code' do let(:user) { create(:user) } let(:project_member) { create(:project_member, :guest, user: user, source: project) } - let(:member_role) { create(:member_role, :guest, download_code: false, members: [project_member], namespace: project.group) } + let(:member_role) { create(:member_role, :guest, read_code: false, members: [project_member], namespace: project.group) } it 'returns hash with project ids as keys and empty array as value' do preloaded = user.preloaded_member_roles_for_projects([project]) @@ -2024,7 +2024,7 @@ it 'does not perform extra queries when asked for projects have already been preloaded' do user.preloaded_member_roles_for_projects([project]) - expect { user.download_code_for?(project) }.not_to exceed_query_limit(0) + expect { user.read_code_for?(project) }.not_to exceed_query_limit(0) end end end diff --git a/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb b/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb index 89a0283e0b65a4..b8b8113cdcf440 100644 --- a/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb +++ b/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb @@ -2,119 +2,105 @@ require 'spec_helper' -RSpec.describe Preloaders::UserMemberRolesInProjectsPreloader do +RSpec.describe Preloaders::UserMemberRolesInProjectsPreloader, feature_category: :authentication_and_authorization do let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project, :private, :in_group) } + let(:project_list) { [project] } + + subject(:result) { described_class.new(projects: project_list, user: user).execute } + context 'when customizable_roles feature is not enabled on project root ancestor' do it 'skips preload' do stub_feature_flags(customizable_roles: false) project_member = create(:project_member, :guest, user: user, source: project) - create(:member_role, :guest, download_code: true, members: [project_member], namespace: project.group) - - result = described_class.new( - projects: [project], - user: user - ).execute + create(:member_role, :guest, read_code: true, members: [project_member], namespace: project.group) expect(result).to eq({}) end end context 'when customizable_roles feature is enabled on project root ancestor' do - context 'when project has custom role with download_code: true' do - context 'when Array of project passed' do - it 'returns the project_id with a value array that includes :download_code' do + context 'when project has custom role' do + context 'when custom role has read_code: true' do + before do project_member = create(:project_member, :guest, user: user, source: project) - create(:member_role, :guest, download_code: true, members: [project_member], namespace: project.group) - - result = described_class.new(projects: [project], user: user).execute - - expect(result).to eq({ project.id => [:download_code] }) + create(:member_role, :guest, members: [project_member], namespace: project.group, read_code: true) end - end - context 'when ActiveRecord::Relation of projects passed' do - it 'returns the project_id with a value array that includes :download_code' do - project_member = create(:project_member, :guest, user: user, source: project) - create( - :member_role, :guest, - download_code: true, - members: [project_member], - namespace: project.group - ) + context 'when Array of project passed' do + it 'returns the project_id with a value array that includes :read_code' do + expect(result).to eq({ project.id => [:read_code] }) + end + end - result = described_class.new(projects: Project.where(id: project.id), user: user).execute + context 'when ActiveRecord::Relation of projects passed' do + let(:project_list) { Project.where(id: project.id) } - expect(result).to eq({ project.id => [:download_code] }) + it 'returns the project_id with a value array that includes :read_code' do + expect(result).to eq({ project.id => [:read_code] }) + end end end end - context 'when project namespace has a custom role with download_code: true' do - it 'returns the project_id with a value array that includes :download_code' do - group_member = create(:group_member, :guest, user: user, source: project.namespace) - create(:member_role, :guest, download_code: true, members: [group_member], namespace: project.group) + context 'when project namespace has a custom role' do + let_it_be(:group_member) { create(:group_member, :guest, user: user, source: project.namespace) } - result = described_class.new(projects: [project], user: user).execute + context 'when custom role with read_code: true' do + it 'returns the project_id with a value array that includes :read_code' do + create(:member_role, :guest, read_code: true, members: [group_member], namespace: project.group) - expect(result).to eq({ project.id => [:download_code] }) + expect(result).to eq({ project.id => [:read_code] }) + end end end context 'when user is a member of the project in multiple ways' do - it 'project value array includes :download_code if any custom roles enable download_code' do - group_member = create(:group_member, :guest, user: user, source: project.group) - project_member = create(:project_member, :guest, user: user, source: project) - create(:member_role, :guest, download_code: false, members: [group_member], namespace: project.group) - create(:member_role, :guest, download_code: true, members: [project_member], namespace: project.group) + let_it_be(:group_member) { create(:group_member, :guest, user: user, source: project.group) } + let_it_be(:project_member) { create(:project_member, :guest, user: user, source: project) } - result = described_class.new(projects: [project], user: user).execute + it 'project value array includes :read_code if any custom roles enable them' do + create(:member_role, :guest, read_code: false, members: [project_member], namespace: project.group) + create(:member_role, :guest, read_code: true, members: [project_member], namespace: project.group) - expect(result).to eq({ project.id => [:download_code] }) + expect(result[project.id]).to match_array([:read_code]) end end context 'when project membership has no custom role' do - it 'returns project id with empty value array' do - project_without_custom_role = create(:project, :private, :in_group) - create(:project_member, :guest, user: user, source: project_without_custom_role) + let_it_be(:project) { create(:project, :private, :in_group) } - result = described_class.new( - projects: [project_without_custom_role], - user: user - ).execute + it 'returns project id with empty value array' do + create(:project_member, :guest, user: user, source: project) - expect(result).to eq(project_without_custom_role.id => []) + expect(result).to eq(project.id => []) end end - context 'when project membership has custom role that does not enable download_code' do + context 'when project membership has custom role that does not enable custom permission' do + let_it_be(:project) { create(:project, :private, :in_group) } + it 'returns project id with empty value array' do - project_without_download_code = create(:project, :private, :in_group) - project_without_download_code_member = create( - :project_member, :guest, + project_without_custom_permission_member = create( + :project_member, + :guest, user: user, - source: project_without_download_code + source: project ) create( :member_role, :guest, - download_code: false, - members: [project_without_download_code_member], - namespace: project_without_download_code.group + read_code: false, + members: [project_without_custom_permission_member], + namespace: project.group ) - result = described_class.new( - projects: [project_without_download_code], - user: user - ).execute - - expect(result).to eq(project_without_download_code.id => []) + expect(result).to eq(project.id => []) end end - context 'when user has custom role that enables download code outside of project hierarchy' do + context 'when user has custom role that enables custom permission outside of project hierarchy' do it 'ignores custom role outside of project hierarchy' do # subgroup is within parent group of project but not above project subgroup = create(:group, parent: project.group) @@ -122,15 +108,10 @@ _custom_role_outside_hierarchy = create( :member_role, :guest, members: [subgroup_member], - download_code: true, + read_code: true, namespace: project.group ) - result = described_class.new( - projects: [project], - user: user - ).execute - expect(result).to eq({ project.id => [] }) end end diff --git a/ee/spec/policies/project_policy_spec.rb b/ee/spec/policies/project_policy_spec.rb index 4e0315e9d8d89d..9f7e0ecd8e4176 100644 --- a/ee/spec/policies/project_policy_spec.rb +++ b/ee/spec/policies/project_policy_spec.rb @@ -2492,21 +2492,21 @@ def expect_private_project_permissions_as_if_non_member ) end - let_it_be(:member_role_download_code_true) do + let_it_be(:member_role_read_code_true) do create( :member_role, :guest, namespace: project.group, - download_code: true + read_code: true ) end - let_it_be(:member_role_download_code_false) do + let_it_be(:member_role_read_code_false) do create( :member_role, :guest, namespace: project.group, - download_code: false + read_code: false ) end @@ -2516,59 +2516,59 @@ def expect_private_project_permissions_as_if_non_member end context 'custom role for parent group' do - context 'custom role allows download code' do + context 'custom role allows read code' do before do - member_role_download_code_true.members << group_member + member_role_read_code_true.members << group_member end - it { is_expected.to be_allowed(:download_code) } + it { is_expected.to be_allowed(:read_code) } end - context 'custom role disallows download code' do + context 'custom role disallows read code' do before do - member_role_download_code_false.members << group_member + member_role_read_code_false.members << group_member end - it { is_expected.to be_disallowed(:download_code) } + it { is_expected.to be_disallowed(:read_code) } end end context 'custom role on project membership' do - context 'custom role allows download code' do + context 'custom role allows read code' do before do - member_role_download_code_true.members << project_member + member_role_read_code_true.members << project_member end - it { is_expected.to be_allowed(:download_code) } + it { is_expected.to be_allowed(:read_code) } end - context 'custom role disallows download code' do + context 'custom role disallows read code' do before do - member_role_download_code_false.members << project_member + member_role_read_code_false.members << project_member end - it { is_expected.to be_disallowed(:download_code) } + it { is_expected.to be_disallowed(:read_code) } end end - context 'multiple custom roles in hierarchy with different download_code values' do + context 'multiple custom roles in hierarchy with different read_code values' do before do - member_role_download_code_true.members << project_member - member_role_download_code_false.members << group_member + member_role_read_code_true.members << project_member + member_role_read_code_false.members << group_member end - # allows download code if any of the custom roles allow it - it { is_expected.to be_allowed(:download_code) } + # allows read code if any of the custom roles allow it + it { is_expected.to be_allowed(:read_code) } end end context 'without customizable_roles feature enabled' do before do stub_feature_flags(customizable_roles: false) - member_role_download_code_true.members << project_member + member_role_read_code_true.members << project_member end - it { is_expected.to be_disallowed(:download_code) } + it { is_expected.to be_disallowed(:read_code) } end end diff --git a/ee/spec/requests/api/member_roles_spec.rb b/ee/spec/requests/api/member_roles_spec.rb index ea76123ee13ac3..c4c5e524ae5664 100644 --- a/ee/spec/requests/api/member_roles_spec.rb +++ b/ee/spec/requests/api/member_roles_spec.rb @@ -2,7 +2,7 @@ require "spec_helper" -RSpec.describe API::MemberRoles, api: true do +RSpec.describe API::MemberRoles, api: true, feature_category: :authentication_and_authorization do include ApiHelpers let_it_be(:owner) { create(:user) } @@ -18,15 +18,21 @@ let_it_be(:child_group) { create :group, parent: group_with_member_roles } let_it_be(:member_role_1) do - create(:member_role, namespace: group_with_member_roles, - base_access_level: ::Gitlab::Access::REPORTER, - download_code: 0) + create( + :member_role, + namespace: group_with_member_roles, + base_access_level: ::Gitlab::Access::REPORTER, + read_code: false + ) end let_it_be(:member_role_2) do - create(:member_role, namespace: group_with_member_roles, - base_access_level: ::Gitlab::Access::REPORTER, - download_code: 1) + create( + :member_role, + namespace: group_with_member_roles, + base_access_level: ::Gitlab::Access::REPORTER, + read_code: true + ) end let_it_be(:group_id) { group_with_member_roles.id } @@ -60,20 +66,22 @@ expect(response).to have_gitlab_http_status(:ok) expect(json_response).to( - match([ - { - "id" => member_role_1.id, - "base_access_level" => ::Gitlab::Access::REPORTER, - "download_code" => false, - "group_id" => group_id - }, - { - "id" => member_role_2.id, - "base_access_level" => ::Gitlab::Access::REPORTER, - "download_code" => true, - "group_id" => group_id - } - ]) + match_array( + [ + { + "id" => member_role_1.id, + "base_access_level" => ::Gitlab::Access::REPORTER, + "read_code" => false, + "group_id" => group_id + }, + { + "id" => member_role_2.id, + "base_access_level" => ::Gitlab::Access::REPORTER, + "read_code" => true, + "group_id" => group_id + } + ] + ) ) end @@ -98,7 +106,9 @@ end describe "POST /groups/:id/member_roles" do - let_it_be(:params) { { base_access_level: 40, download_code: 1 } } + let_it_be(:params) do + { base_access_level: ::Gitlab::Access::MAINTAINER, read_code: true } + end subject { post api("/groups/#{group_id}/member_roles", current_user), params: params } @@ -138,12 +148,12 @@ aggregate_failures "testing response" do expect(response).to have_gitlab_http_status(:created) expect(json_response['base_access_level']).to eq(::Gitlab::Access::MAINTAINER) - expect(json_response['download_code']).to eq(true) + expect(json_response['read_code']).to eq(true) end end context "when params are missing" do - let(:params) { { download_code: 0 } } + let(:params) { { read_code: false } } it "returns a 400 error when params are missing" do subject @@ -154,7 +164,7 @@ end context "when params are invalid" do - let(:params) { { base_access_level: 1, download_code: 1 } } + let(:params) { { base_access_level: 1 } } it "returns a 400 error when params are invalid" do subject -- GitLab From d1b5511907082ea53b9bfc331b28a7d7a323f4f0 Mon Sep 17 00:00:00 2001 From: Jessie Young Date: Wed, 7 Dec 2022 12:12:39 -0800 Subject: [PATCH 2/4] Fix N+1 --- spec/requests/projects/ml/experiments_controller_spec.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/spec/requests/projects/ml/experiments_controller_spec.rb b/spec/requests/projects/ml/experiments_controller_spec.rb index 414748c0804adc..23647293c1afe9 100644 --- a/spec/requests/projects/ml/experiments_controller_spec.rb +++ b/spec/requests/projects/ml/experiments_controller_spec.rb @@ -17,7 +17,6 @@ let(:params) { basic_params } let(:ff_value) { true } - let(:threshold) { 5 } let(:project) { project_with_feature } let(:basic_params) { { namespace_id: project.namespace.to_param, project_id: project } } @@ -47,12 +46,12 @@ expect(response).to render_template('projects/ml/experiments/index') end - it 'does not perform N+1 sql queries' do - control_count = ActiveRecord::QueryRecorder.new { list_experiments } + it 'does not perform N+1 sql queries', :use_sql_query_cache do + control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { list_experiments } create_list(:ml_experiments, 2, project: project, user: user) - expect { list_experiments }.not_to exceed_all_query_limit(control_count).with_threshold(threshold) + expect { list_experiments }.not_to exceed_all_query_limit(control_count) end context 'when :ml_experiment_tracking is disabled for the project' do -- GitLab From 026751c7ad6e36bba4d31c4ee203a7ebeb793db6 Mon Sep 17 00:00:00 2001 From: Jessie Young Date: Thu, 8 Dec 2022 21:09:25 -0800 Subject: [PATCH 3/4] Clean up project member references in specs --- ...member_roles_in_projects_preloader_spec.rb | 27 ++++++++----------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb b/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb index b8b8113cdcf440..bbe98ee3721ed5 100644 --- a/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb +++ b/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb @@ -5,6 +5,7 @@ RSpec.describe Preloaders::UserMemberRolesInProjectsPreloader, feature_category: :authentication_and_authorization do let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project, :private, :in_group) } + let_it_be(:project_member) { create(:project_member, :guest, user: user, source: project) } let(:project_list) { [project] } @@ -13,7 +14,6 @@ context 'when customizable_roles feature is not enabled on project root ancestor' do it 'skips preload' do stub_feature_flags(customizable_roles: false) - project_member = create(:project_member, :guest, user: user, source: project) create(:member_role, :guest, read_code: true, members: [project_member], namespace: project.group) expect(result).to eq({}) @@ -22,12 +22,11 @@ context 'when customizable_roles feature is enabled on project root ancestor' do context 'when project has custom role' do - context 'when custom role has read_code: true' do - before do - project_member = create(:project_member, :guest, user: user, source: project) - create(:member_role, :guest, members: [project_member], namespace: project.group, read_code: true) - end + let_it_be(:member_role) do + create(:member_role, :guest, members: [project_member], namespace: project.group, read_code: true) + end + context 'when custom role has read_code: true' do context 'when Array of project passed' do it 'returns the project_id with a value array that includes :read_code' do expect(result).to eq({ project.id => [:read_code] }) @@ -44,21 +43,19 @@ end end - context 'when project namespace has a custom role' do + context 'when project namespace has a custom role with read_code: true' do let_it_be(:group_member) { create(:group_member, :guest, user: user, source: project.namespace) } + let_it_be(:member_role) do + create(:member_role, :guest, read_code: true, members: [group_member], namespace: project.group) + end - context 'when custom role with read_code: true' do - it 'returns the project_id with a value array that includes :read_code' do - create(:member_role, :guest, read_code: true, members: [group_member], namespace: project.group) - - expect(result).to eq({ project.id => [:read_code] }) - end + it 'returns the project_id with a value array that includes :read_code' do + expect(result).to eq({ project.id => [:read_code] }) end end context 'when user is a member of the project in multiple ways' do let_it_be(:group_member) { create(:group_member, :guest, user: user, source: project.group) } - let_it_be(:project_member) { create(:project_member, :guest, user: user, source: project) } it 'project value array includes :read_code if any custom roles enable them' do create(:member_role, :guest, read_code: false, members: [project_member], namespace: project.group) @@ -72,8 +69,6 @@ let_it_be(:project) { create(:project, :private, :in_group) } it 'returns project id with empty value array' do - create(:project_member, :guest, user: user, source: project) - expect(result).to eq(project.id => []) end end -- GitLab From 7b8f334d05bf84c07d0ed1f705a6772b25faabaf Mon Sep 17 00:00:00 2001 From: Jessie Young Date: Thu, 8 Dec 2022 21:22:53 -0800 Subject: [PATCH 4/4] Simplify N+1 fix --- spec/requests/projects/ml/experiments_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/projects/ml/experiments_controller_spec.rb b/spec/requests/projects/ml/experiments_controller_spec.rb index 23647293c1afe9..f35f93b1e6c885 100644 --- a/spec/requests/projects/ml/experiments_controller_spec.rb +++ b/spec/requests/projects/ml/experiments_controller_spec.rb @@ -46,7 +46,7 @@ expect(response).to render_template('projects/ml/experiments/index') end - it 'does not perform N+1 sql queries', :use_sql_query_cache do + it 'does not perform N+1 sql queries' do control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { list_experiments } create_list(:ml_experiments, 2, project: project, user: user) -- GitLab