diff --git a/.gitlab/ci/rules.gitlab-ci.yml b/.gitlab/ci/rules.gitlab-ci.yml index 5c9be5b1e9f..4d083541b43 100644 --- a/.gitlab/ci/rules.gitlab-ci.yml +++ b/.gitlab/ci/rules.gitlab-ci.yml @@ -119,7 +119,7 @@ - ".gitlab-ci.yml" - ".gitlab/ci/**/*.yml" - "lib/gitlab/ci/templates/**/*.yml" - - "{,ee/}changelogs/**/*.yml" + - "{,ee/,jh/}changelogs/**/*.yml" .docs-patterns: &docs-patterns - ".gitlab/route-map.yml" @@ -138,7 +138,7 @@ - "config/webpack.config.js" - "config/**/*.js" - "vendor/assets/**/*" - - "{,ee/}app/assets/**/*" + - "{,ee/,jh/}app/assets/**/*" .frontend-patterns: &frontend-patterns - "{package.json,yarn.lock}" @@ -148,45 +148,45 @@ - "Dockerfile.assets" - "config/**/*.js" - "vendor/assets/**/*" - - "{,ee/}{app/assets,app/helpers,app/presenters,app/views,locale,public,symbol}/**/*" + - "{,ee/,jh/}{app/assets,app/helpers,app/presenters,app/views,locale,public,symbol}/**/*" .backend-patterns: &backend-patterns - "Gemfile{,.lock}" - "Rakefile" - "config.ru" # List explicitly all the app/ dirs that are backend (i.e. all except app/assets). - - "{,ee/}{app/channels,app/controllers,app/finders,app/graphql,app/helpers,app/mailers,app/models,app/policies,app/presenters,app/serializers,app/services,app/uploaders,app/validators,app/views,app/workers}/**/*" - - "{,ee/}{bin,cable,config,db,lib}/**/*" - - "{,ee/}spec/**/*.rb" + - "{,ee/,jh/}{app/channels,app/controllers,app/finders,app/graphql,app/helpers,app/mailers,app/models,app/policies,app/presenters,app/serializers,app/services,app/uploaders,app/validators,app/views,app/workers}/**/*" + - "{,ee/,jh/}{bin,cable,config,db,lib}/**/*" + - "{,ee/,jh/}spec/**/*.rb" # CI changes - ".gitlab-ci.yml" - ".gitlab/ci/**/*" - "*_VERSION" .db-patterns: &db-patterns - - "{,ee/}{,spec/}{db,migrations}/**/*" - - "{,ee/}{,spec/}lib/{,ee/}gitlab/database/**/*" - - "{,ee/}{,spec/}lib/{,ee/}gitlab/database{,_spec}.rb" - - "{,ee/}{,spec/}lib/{,ee/}gitlab/background_migration/**/*" - - "{,ee/}{,spec/}lib/{,ee/}gitlab/background_migration{,_spec}.rb" - - "{,ee/}spec/support/helpers/database/**/*" + - "{,ee/,jh/}{,spec/}{db,migrations}/**/*" + - "{,ee/,jh/}{,spec/}lib/{,ee/,jh/}gitlab/database/**/*" + - "{,ee/,jh/}{,spec/}lib/{,ee/,jh/}gitlab/database{,_spec}.rb" + - "{,ee/,jh/}{,spec/}lib/{,ee/,jh/}gitlab/background_migration/**/*" + - "{,ee/,jh/}{,spec/}lib/{,ee/,jh/}gitlab/background_migration{,_spec}.rb" + - "{,ee/,jh/}spec/support/helpers/database/**/*" - "config/prometheus/common_metrics.yml" # Used by Gitlab::DatabaseImporters::CommonMetrics::Importer - - "{,ee/}app/models/project_statistics.rb" # Used to calculate sizes in migration specs + - "{,ee/,jh/}app/models/project_statistics.rb" # Used to calculate sizes in migration specs # CI changes - ".gitlab-ci.yml" - ".gitlab/ci/**/*" .db-library-patterns: &db-library-patterns - - "{,ee/}{,spec/}lib/{,ee/}gitlab/database/**/*" - - "{,ee/}{,spec/}lib/{,ee/}gitlab/database{,_spec}.rb" - - "{,ee/}spec/support/helpers/database/**/*" + - "{,ee/,jh/}{,spec/}lib/{,ee/,jh/}gitlab/database/**/*" + - "{,ee/,jh/}{,spec/}lib/{,ee/,jh/}gitlab/database{,_spec}.rb" + - "{,ee/,jh/}spec/support/helpers/database/**/*" .backstage-patterns: &backstage-patterns - "Dangerfile" - "danger/**/*" - - "{,ee/}fixtures/**/*" - - "{,ee/}rubocop/**/*" - - "{,ee/}spec/**/*" + - "{,ee/,jh/}fixtures/**/*" + - "{,ee/,jh/}rubocop/**/*" + - "{,ee/,jh/}spec/**/*" - "{,spec/}tooling/**/*" .code-patterns: &code-patterns @@ -206,7 +206,7 @@ - "Rakefile" - "tests.yml" - "config.ru" - - "{,ee/}{app,bin,config,db,haml_lint,lib,locale,public,scripts,symbol,vendor}/**/*" + - "{,ee/,jh/}{app,bin,config,db,haml_lint,lib,locale,public,scripts,symbol,vendor}/**/*" - "doc/api/graphql/reference/*" # Files in this folder are auto-generated - "data/whats_new/*.yml" @@ -231,15 +231,15 @@ - "Rakefile" - "tests.yml" - "config.ru" - - "{,ee/}{app,bin,config,db,haml_lint,lib,locale,public,scripts,symbol,vendor}/**/*" + - "{,ee/,jh/}{app,bin,config,db,haml_lint,lib,locale,public,scripts,symbol,vendor}/**/*" - "doc/api/graphql/reference/*" # Files in this folder are auto-generated - "data/whats_new/*.yml" # Backstage changes - "Dangerfile" - "danger/**/*" - - "{,ee/}fixtures/**/*" - - "{,ee/}rubocop/**/*" - - "{,ee/}spec/**/*" + - "{,ee/,jh/}fixtures/**/*" + - "{,ee/,jh/}rubocop/**/*" + - "{,ee/,jh/}spec/**/*" - "{,spec/}tooling/**/*" .code-qa-patterns: &code-qa-patterns @@ -259,7 +259,7 @@ - "Rakefile" - "tests.yml" - "config.ru" - - "{,ee/}{app,bin,config,db,haml_lint,lib,locale,public,scripts,symbol,vendor}/**/*" + - "{,ee/,jh/}{app,bin,config,db,haml_lint,lib,locale,public,scripts,symbol,vendor}/**/*" - "doc/api/graphql/reference/*" # Files in this folder are auto-generated - "data/whats_new/*.yml" # QA changes @@ -283,15 +283,15 @@ - "Rakefile" - "tests.yml" - "config.ru" - - "{,ee/}{app,bin,config,db,haml_lint,lib,locale,public,scripts,symbol,vendor}/**/*" + - "{,ee/,jh/}{app,bin,config,db,haml_lint,lib,locale,public,scripts,symbol,vendor}/**/*" - "doc/api/graphql/reference/*" # Files in this folder are auto-generated - "data/whats_new/*.yml" # Backstage changes - "Dangerfile" - "danger/**/*" - - "{,ee/}fixtures/**/*" - - "{,ee/}rubocop/**/*" - - "{,ee/}spec/**/*" + - "{,ee/,jh/}fixtures/**/*" + - "{,ee/,jh/}rubocop/**/*" + - "{,ee/,jh/}spec/**/*" - "{,spec/}tooling/**/*" # QA changes - ".dockerignore" diff --git a/app/assets/javascripts/tracking.js b/app/assets/javascripts/tracking.js index d2e69bc06cf..3b1af5eba93 100644 --- a/app/assets/javascripts/tracking.js +++ b/app/assets/javascripts/tracking.js @@ -25,6 +25,10 @@ const DEFAULT_SNOWPLOW_OPTIONS = { formTracking: false, linkClickTracking: false, pageUnloadTimer: 10, + formTrackingConfig: { + forms: { allow: [] }, + fields: { allow: [] }, + }, }; const addExperimentContext = (opts) => { @@ -156,13 +160,18 @@ export default class Tracking { static enableFormTracking(config, contexts = []) { if (!this.enabled()) return; - if (!config?.forms?.whitelist?.length && !config?.fields?.whitelist?.length) { + if (!Array.isArray(config?.forms?.allow) && !Array.isArray(config?.fields?.allow)) { // eslint-disable-next-line @gitlab/require-i18n-strings - throw new Error('Unable to enable form event tracking without whitelist rules.'); + throw new Error('Unable to enable form event tracking without allow rules.'); } contexts.unshift(STANDARD_CONTEXT); - const enabler = () => window.snowplow('enableFormTracking', config, contexts); + const mappedConfig = { + forms: { whitelist: config.forms?.allow || [] }, + fields: { whitelist: config.fields?.allow || [] }, + }; + + const enabler = () => window.snowplow('enableFormTracking', mappedConfig, contexts); if (document.readyState !== 'loading') enabler(); else document.addEventListener('DOMContentLoaded', enabler); @@ -207,11 +216,13 @@ export function initUserTracking() { export function initDefaultTrackers() { if (!Tracking.enabled()) return; + const opts = { ...DEFAULT_SNOWPLOW_OPTIONS, ...window.snowplowOptions }; + window.snowplow('enableActivityTracking', 30, 30); // must be after enableActivityTracking window.snowplow('trackPageView', null, [STANDARD_CONTEXT]); - if (window.snowplowOptions.formTracking) window.snowplow('enableFormTracking'); + if (window.snowplowOptions.formTracking) Tracking.enableFormTracking(opts.formTrackingConfig); if (window.snowplowOptions.linkClickTracking) window.snowplow('enableLinkClickTracking'); Tracking.bindDocument(); diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb index c2ac56ccc63..fa0737d5b13 100644 --- a/app/controllers/groups/group_members_controller.rb +++ b/app/controllers/groups/group_members_controller.rb @@ -25,7 +25,6 @@ class Groups::GroupMembersController < Groups::ApplicationController helper_method :can_manage_members? def index - preload_max_access @sort = params[:sort].presence || sort_value_name @members = GroupMembersFinder @@ -54,14 +53,6 @@ class Groups::GroupMembersController < Groups::ApplicationController private - def preload_max_access - return unless current_user - - # this allows the can? against admin type queries in this action to - # only perform the query once, even if it is cached - current_user.max_access_for_group[@group.id] = @group.max_member_access(current_user) - end - def can_manage_members? strong_memoize(:can_manage_members) do can?(current_user, :admin_group_member, @group) diff --git a/app/models/group.rb b/app/models/group.rb index da795651c63..deaa0ebc895 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -17,6 +17,7 @@ class Group < Namespace include GroupAPICompatibility include EachBatch include HasTimelogsReport + include BulkMemberAccessLoad ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT = 10 @@ -569,24 +570,8 @@ class Group < Namespace def max_member_access_for_user(user, only_concrete_membership: false) return GroupMember::NO_ACCESS unless user return GroupMember::OWNER if user.can_admin_all_resources? && !only_concrete_membership - # Use the preloaded value that exists instead of performing the db query again(cached or not). - # Groups::GroupMembersController#preload_max_access makes use of this by - # calling Group#max_member_access. This helps when we have a process - # that may query this multiple times from the outside through a policy query - # like the GroupPolicy#lookup_access_level! does as a condition for any role - return user.max_access_for_group[id] if user.max_access_for_group[id] - max_member_access(user) - end - - def max_member_access(user) - max_member_access = members_with_parents - .where(user_id: user) - .reorder(access_level: :desc) - .first - &.access_level - - max_member_access || GroupMember::NO_ACCESS + max_member_access([user.id])[user.id] end def mattermost_team_params @@ -730,6 +715,12 @@ class Group < Namespace private + def max_member_access(user_ids) + max_member_access_for_resource_ids(User, user_ids) do |user_ids| + members_with_parents.where(user_id: user_ids).group(:user_id).maximum(:access_level) + end + end + def update_two_factor_requirement return unless saved_change_to_require_two_factor_authentication? || saved_change_to_two_factor_grace_period? diff --git a/app/models/user.rb b/app/models/user.rb index c3efb3b71ce..a7964cf0001 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -99,12 +99,6 @@ class User < ApplicationRecord # Virtual attribute for impersonator attr_accessor :impersonator - attr_writer :max_access_for_group - - def max_access_for_group - @max_access_for_group ||= {} - end - # # Relations # diff --git a/doc/user/project/merge_requests/code_quality.md b/doc/user/project/merge_requests/code_quality.md index 284d66dd591..7ffd4871f09 100644 --- a/doc/user/project/merge_requests/code_quality.md +++ b/doc/user/project/merge_requests/code_quality.md @@ -527,7 +527,7 @@ This can be due to multiple reasons: - You just added the Code Quality job in your `.gitlab-ci.yml`. The report does not have anything to compare to yet, so no information can be displayed. It only displays after future merge requests have something to compare to. -- Your pipeline is not set to run the code quality job on your default branch. If there is no report generated from the default branch, your MR branch reports have nothing to compare to. +- Your pipeline is not set to run the code quality job on your target branch. If there is no report generated from the target branch, your MR branch reports have nothing to compare to. - If no [degradation or error is detected](https://docs.codeclimate.com/docs/maintainability#section-checks), nothing is displayed. - The [`artifacts:expire_in`](../../../ci/yaml/README.md#artifactsexpire_in) CI/CD diff --git a/qa/qa.rb b/qa/qa.rb index fafa428be41..bf86fc5faec 100644 --- a/qa/qa.rb +++ b/qa/qa.rb @@ -73,7 +73,9 @@ module QA autoload :Issue, 'qa/resource/issue' autoload :ProjectIssueNote, 'qa/resource/project_issue_note' autoload :Project, 'qa/resource/project' - autoload :Label, 'qa/resource/label' + autoload :LabelBase, 'qa/resource/label_base' + autoload :ProjectLabel, 'qa/resource/project_label' + autoload :GroupLabel, 'qa/resource/group_label' autoload :MergeRequest, 'qa/resource/merge_request' autoload :ProjectImportedFromGithub, 'qa/resource/project_imported_from_github' autoload :MergeRequestFromFork, 'qa/resource/merge_request_from_fork' diff --git a/qa/qa/resource/group_base.rb b/qa/qa/resource/group_base.rb index 19df390a10f..025d98f50e0 100644 --- a/qa/qa/resource/group_base.rb +++ b/qa/qa/resource/group_base.rb @@ -14,6 +14,29 @@ module QA attribute :name attribute :full_path + # Get group labels + # + # @return [Array] + def labels + parse_body(api_get_from("#{api_get_path}/labels")).map do |label| + GroupLabel.new.tap do |resource| + resource.api_client = api_client + resource.group = self + resource.id = label[:id] + resource.title = label[:name] + resource.description = label[:description] + resource.color = label[:color] + end + end + end + + # API get path + # + # @return [String] + def api_get_path + raise NotImplementedError + end + # API post path # # @return [String] diff --git a/qa/qa/resource/group_label.rb b/qa/qa/resource/group_label.rb new file mode 100644 index 00000000000..7b700c122ce --- /dev/null +++ b/qa/qa/resource/group_label.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module QA + module Resource + class GroupLabel < LabelBase + attribute :group do + Group.fabricate! do |resource| + resource.name = 'group-with-label' + end + end + + def fabricate! + raise NotImplementedError + end + + def api_post_path + "/groups/#{group.id}/labels" + end + + def api_get_path + "/groups/#{group.id}/labels/#{id}" + end + end + end +end diff --git a/qa/qa/resource/label.rb b/qa/qa/resource/label.rb deleted file mode 100644 index 6b0b0184130..00000000000 --- a/qa/qa/resource/label.rb +++ /dev/null @@ -1,61 +0,0 @@ -# frozen_string_literal: true - -require 'securerandom' - -module QA - module Resource - class Label < Base - attr_accessor :description, :color - - attribute :id - attribute :title - - attribute :project do - Project.fabricate! do |resource| - resource.name = 'project-with-label' - end - end - - def initialize - @title = "qa-test-#{SecureRandom.hex(8)}" - @description = 'This is a test label' - @color = '#0033CC' - end - - def fabricate! - project.visit! - - Page::Project::Menu.perform(&:go_to_labels) - Page::Label::Index.perform(&:click_new_label_button) - - Page::Label::New.perform do |new_page| - new_page.fill_title(@title) - new_page.fill_description(@description) - new_page.fill_color(@color) - new_page.click_label_create_button - end - end - - def resource_web_url(resource) - super - rescue ResourceURLMissingError - # this particular resource does not expose a web_url property - end - - def api_get_path - raise NotImplementedError, "The Labels API doesn't expose a single-resource endpoint so this method cannot be properly implemented." - end - - def api_post_path - "/projects/#{project.id}/labels" - end - - def api_post_body - { - color: @color, - name: @title - } - end - end - end -end diff --git a/qa/qa/resource/label_base.rb b/qa/qa/resource/label_base.rb new file mode 100644 index 00000000000..14ddd0809ea --- /dev/null +++ b/qa/qa/resource/label_base.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +require 'securerandom' + +module QA + module Resource + # Base label class for GroupLabel and ProjectLabel + # + class LabelBase < Base + attr_accessor :title, :description, :color + + attribute :id + attribute :description_html + attribute :text_color + attribute :subscribed + + def initialize + @title = "qa-test-#{SecureRandom.hex(8)}" + @description = 'This is a test label' + @color = '#0033CC' + end + + def fabricate! + Page::Label::Index.perform(&:click_new_label_button) + Page::Label::New.perform do |new_page| + new_page.fill_title(title) + new_page.fill_description(description) + new_page.fill_color(color) + new_page.click_label_create_button + end + end + + # Resource web url + # + # @param [Hash] resource + # @return [String] + def resource_web_url(resource) + super + rescue ResourceURLMissingError + # this particular resource does not expose a web_url property + end + + # Params for label creation + # + # @return [Hash] + def api_post_body + { + name: title, + color: color, + description: description + } + end + + # Object comparison + # + # @param [QA::Resource::GroupBase] other + # @return [Boolean] + def ==(other) + other.is_a?(LabelBase) && comparable_label == other.comparable_label + end + + # Override inspect for a better rspec failure diff output + # + # @return [String] + def inspect + JSON.pretty_generate(comparable_label) + end + + # protected + + # Return subset of fields for comparing groups + # + # @return [Hash] + def comparable_label + reload! unless api_response + + api_response.slice( + :name, + :description, + :description_html, + :color, + :text_color, + :subscribed + ) + end + end + end +end diff --git a/qa/qa/resource/project_label.rb b/qa/qa/resource/project_label.rb new file mode 100644 index 00000000000..6b2943a801e --- /dev/null +++ b/qa/qa/resource/project_label.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module QA + module Resource + class ProjectLabel < LabelBase + attribute :project do + Project.fabricate! do |resource| + resource.name = 'project-with-label' + end + end + + def fabricate! + project.visit! + Page::Project::Menu.perform(&:go_to_labels) + + super + end + + def api_post_path + "/projects/#{project.id}/labels" + end + + def api_get_path + "/projects/#{project.id}/labels/#{id}" + end + end + end +end diff --git a/qa/qa/specs/features/browser_ui/1_manage/group/bulk_import_group_spec.rb b/qa/qa/specs/features/browser_ui/1_manage/group/bulk_import_group_spec.rb index ccf13ac01da..6044409ea40 100644 --- a/qa/qa/specs/features/browser_ui/1_manage/group/bulk_import_group_spec.rb +++ b/qa/qa/specs/features/browser_ui/1_manage/group/bulk_import_group_spec.rb @@ -37,8 +37,9 @@ module QA let(:imported_group) do Resource::Group.new.tap do |group| group.api_client = api_client + group.sandbox = sandbox group.path = source_group.path - end + end.reload! end let(:imported_subgroup) do @@ -46,7 +47,7 @@ module QA group.api_client = api_client group.sandbox = imported_group group.path = subgroup.path - end + end.reload! end def staging? @@ -59,6 +60,17 @@ module QA end before do + Resource::GroupLabel.fabricate_via_api! do |label| + label.api_client = api_client + label.group = source_group + label.title = "source-group-#{SecureRandom.hex(4)}" + end + Resource::GroupLabel.fabricate_via_api! do |label| + label.api_client = api_client + label.group = subgroup + label.title = "subgroup-#{SecureRandom.hex(4)}" + end + sandbox.add_member(user, Resource::Members::AccessLevel::MAINTAINER) source_group.add_member(user, Resource::Members::AccessLevel::MAINTAINER) @@ -72,15 +84,22 @@ module QA testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/1785', exclude: { job: ['ce:relative_url', 'ee:relative_url'] }, issue_1: "https://gitlab.com/gitlab-org/gitlab/-/issues/330344", - issue_2: "https://gitlab.com/gitlab-org/gitlab/-/issues/331252" + issue_2: "https://gitlab.com/gitlab-org/gitlab/-/issues/331252", + issue_3: "https://gitlab.com/gitlab-org/gitlab/-/issues/331704" ) do Page::Group::BulkImport.perform do |import_page| import_page.import_group(source_group.path, sandbox.path) aggregate_failures do expect(import_page).to have_imported_group(source_group.path, wait: 120) + expect(imported_group).to eq(source_group) expect(imported_subgroup).to eq(subgroup) + + # TODO: Improve validation logic with some potential retry mechanism built in to custom rspec matcher + # https://gitlab.com/gitlab-org/gitlab/-/issues/331704 + # expect(imported_group.labels).to include(*source_group.labels) + expect(imported_subgroup.labels).to include(*subgroup.labels) end end end diff --git a/qa/qa/specs/features/browser_ui/3_create/merge_request/create_merge_request_spec.rb b/qa/qa/specs/features/browser_ui/3_create/merge_request/create_merge_request_spec.rb index b998ad24358..082d001b716 100644 --- a/qa/qa/specs/features/browser_ui/3_create/merge_request/create_merge_request_spec.rb +++ b/qa/qa/specs/features/browser_ui/3_create/merge_request/create_merge_request_spec.rb @@ -16,7 +16,11 @@ module QA Flow::Login.sign_in end - it 'creates a basic merge request', :smoke, testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/1850' do + it( + 'creates a basic merge request', + :smoke, + testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/1850' + ) do Resource::MergeRequest.fabricate_via_browser_ui! do |merge_request| merge_request.project = project merge_request.title = merge_request_title @@ -29,14 +33,17 @@ module QA end end - it 'creates a merge request with a milestone and label', testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/514' do + it( + 'creates a merge request with a milestone and label', + testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/514' + ) do gitlab_account_username = "@#{Runtime::User.username}" milestone = Resource::ProjectMilestone.fabricate_via_api! do |milestone| milestone.project = project end - label = Resource::Label.fabricate_via_api! do |label| + label = Resource::ProjectLabel.fabricate_via_api! do |label| label.project = project label.title = 'label' end diff --git a/spec/controllers/groups/group_members_controller_spec.rb b/spec/controllers/groups/group_members_controller_spec.rb index b666f73110a..a3e0c9f9ca4 100644 --- a/spec/controllers/groups/group_members_controller_spec.rb +++ b/spec/controllers/groups/group_members_controller_spec.rb @@ -32,14 +32,6 @@ RSpec.describe Groups::GroupMembersController do sign_in(user) end - it 'assigns max_access_for_group' do - allow(controller).to receive(:current_user).and_return(user) - - get :index, params: { group_id: group } - - expect(user.max_access_for_group[group.id]).to eq(Gitlab::Access::OWNER) - end - it 'assigns invited members' do get :index, params: { group_id: group } diff --git a/spec/frontend/tracking_spec.js b/spec/frontend/tracking_spec.js index dd4c8198b72..9fd5eab6381 100644 --- a/spec/frontend/tracking_spec.js +++ b/spec/frontend/tracking_spec.js @@ -9,6 +9,7 @@ describe('Tracking', () => { let snowplowSpy; let bindDocumentSpy; let trackLoadEventsSpy; + let enableFormTracking; beforeEach(() => { getExperimentData.mockReturnValue(undefined); @@ -38,6 +39,10 @@ describe('Tracking', () => { formTracking: false, linkClickTracking: false, pageUnloadTimer: 10, + formTrackingConfig: { + fields: { allow: [] }, + forms: { allow: [] }, + }, }); }); }); @@ -46,6 +51,9 @@ describe('Tracking', () => { beforeEach(() => { bindDocumentSpy = jest.spyOn(Tracking, 'bindDocument').mockImplementation(() => null); trackLoadEventsSpy = jest.spyOn(Tracking, 'trackLoadEvents').mockImplementation(() => null); + enableFormTracking = jest + .spyOn(Tracking, 'enableFormTracking') + .mockImplementation(() => null); }); it('should activate features based on what has been enabled', () => { @@ -59,10 +67,11 @@ describe('Tracking', () => { ...window.snowplowOptions, formTracking: true, linkClickTracking: true, + formTrackingConfig: { forms: { whitelist: ['foo'] }, fields: { whitelist: ['bar'] } }, }; initDefaultTrackers(); - expect(snowplowSpy).toHaveBeenCalledWith('enableFormTracking'); + expect(enableFormTracking).toHaveBeenCalledWith(window.snowplowOptions.formTrackingConfig); expect(snowplowSpy).toHaveBeenCalledWith('enableLinkClickTracking'); }); @@ -157,25 +166,22 @@ describe('Tracking', () => { describe('.enableFormTracking', () => { it('tells snowplow to enable form tracking', () => { - const config = { forms: { whitelist: [''] }, fields: { whitelist: [''] } }; + const config = { forms: { allow: ['form-class1'] }, fields: { allow: ['input-class1'] } }; Tracking.enableFormTracking(config, ['_passed_context_']); - expect(snowplowSpy).toHaveBeenCalledWith('enableFormTracking', config, [ - { data: { source: 'gitlab-javascript' }, schema: undefined }, - '_passed_context_', - ]); + expect(snowplowSpy).toHaveBeenCalledWith( + 'enableFormTracking', + { forms: { whitelist: ['form-class1'] }, fields: { whitelist: ['input-class1'] } }, + [{ data: { source: 'gitlab-javascript' }, schema: undefined }, '_passed_context_'], + ); }); - it('throws an error if no whitelist rules are provided', () => { - const expectedError = new Error( - 'Unable to enable form event tracking without whitelist rules.', - ); + it('throws an error if no allow rules are provided', () => { + const expectedError = new Error('Unable to enable form event tracking without allow rules.'); expect(() => Tracking.enableFormTracking()).toThrow(expectedError); - expect(() => Tracking.enableFormTracking({ fields: { whitelist: [] } })).toThrow( - expectedError, - ); - expect(() => Tracking.enableFormTracking({ fields: { whitelist: [1] } })).not.toThrow( + expect(() => Tracking.enableFormTracking({ fields: { allow: true } })).toThrow(expectedError); + expect(() => Tracking.enableFormTracking({ fields: { allow: [] } })).not.toThrow( expectedError, ); }); diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 5cc5c4d86d6..e22ae41be7f 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1093,167 +1093,151 @@ RSpec.describe Group do it { expect(subject.parent).to be_kind_of(described_class) } end - context "with member access" do + describe '#max_member_access_for_user' do let_it_be(:group_user) { create(:user) } - describe '#max_member_access_for_user' do + context 'with user in the group' do + before do + group.add_owner(group_user) + end + + it 'returns correct access level' do + expect(group.max_member_access_for_user(group_user)).to eq(Gitlab::Access::OWNER) + end + end + + context 'when user is nil' do + it 'returns NO_ACCESS' do + expect(group.max_member_access_for_user(nil)).to eq(Gitlab::Access::NO_ACCESS) + end + end + + context 'evaluating admin access level' do + let_it_be(:admin) { create(:admin) } + + context 'when admin mode is enabled', :enable_admin_mode do + it 'returns OWNER by default' do + expect(group.max_member_access_for_user(admin)).to eq(Gitlab::Access::OWNER) + end + end + + context 'when admin mode is disabled' do + it 'returns NO_ACCESS' do + expect(group.max_member_access_for_user(admin)).to eq(Gitlab::Access::NO_ACCESS) + end + end + + it 'returns NO_ACCESS when only concrete membership should be considered' do + expect(group.max_member_access_for_user(admin, only_concrete_membership: true)) + .to eq(Gitlab::Access::NO_ACCESS) + end + end + + context 'group shared with another group' do + let_it_be(:parent_group_user) { create(:user) } + let_it_be(:child_group_user) { create(:user) } + + let_it_be(:group_parent) { create(:group, :private) } + let_it_be(:group) { create(:group, :private, parent: group_parent) } + let_it_be(:group_child) { create(:group, :private, parent: group) } + + let_it_be(:shared_group_parent) { create(:group, :private) } + let_it_be(:shared_group) { create(:group, :private, parent: shared_group_parent) } + let_it_be(:shared_group_child) { create(:group, :private, parent: shared_group) } + + before do + group_parent.add_owner(parent_group_user) + group.add_owner(group_user) + group_child.add_owner(child_group_user) + + create(:group_group_link, { shared_with_group: group, + shared_group: shared_group, + group_access: GroupMember::DEVELOPER }) + end + context 'with user in the group' do + it 'returns correct access level' do + expect(shared_group_parent.max_member_access_for_user(group_user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group.max_member_access_for_user(group_user)).to eq(Gitlab::Access::DEVELOPER) + expect(shared_group_child.max_member_access_for_user(group_user)).to eq(Gitlab::Access::DEVELOPER) + end + + context 'with lower group access level than max access level for share' do + let(:user) { create(:user) } + + it 'returns correct access level' do + group.add_reporter(user) + + expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::REPORTER) + expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::REPORTER) + end + end + end + + context 'with user in the parent group' do + it 'returns correct access level' do + expect(shared_group_parent.max_member_access_for_user(parent_group_user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group.max_member_access_for_user(parent_group_user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group_child.max_member_access_for_user(parent_group_user)).to eq(Gitlab::Access::NO_ACCESS) + end + end + + context 'with user in the child group' do + it 'returns correct access level' do + expect(shared_group_parent.max_member_access_for_user(child_group_user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group.max_member_access_for_user(child_group_user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group_child.max_member_access_for_user(child_group_user)).to eq(Gitlab::Access::NO_ACCESS) + end + end + + context 'unrelated project owner' do + let(:common_id) { [Project.maximum(:id).to_i, Namespace.maximum(:id).to_i].max + 999 } + let!(:group) { create(:group, id: common_id) } + let!(:unrelated_project) { create(:project, id: common_id) } + let(:user) { unrelated_project.owner } + + it 'returns correct access level' do + expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + end + end + + context 'user without accepted access request' do + let!(:user) { create(:user) } + before do - group.add_owner(group_user) + create(:group_member, :developer, :access_request, user: user, group: group) end it 'returns correct access level' do - expect(group.max_member_access_for_user(group_user)).to eq(Gitlab::Access::OWNER) - end - end - - context 'when user is nil' do - it 'returns NO_ACCESS' do - expect(group.max_member_access_for_user(nil)).to eq(Gitlab::Access::NO_ACCESS) - end - end - - context 'evaluating admin access level' do - let_it_be(:admin) { create(:admin) } - - context 'when admin mode is enabled', :enable_admin_mode do - it 'returns OWNER by default' do - expect(group.max_member_access_for_user(admin)).to eq(Gitlab::Access::OWNER) - end - end - - context 'when admin mode is disabled' do - it 'returns NO_ACCESS' do - expect(group.max_member_access_for_user(admin)).to eq(Gitlab::Access::NO_ACCESS) - end - end - - it 'returns NO_ACCESS when only concrete membership should be considered' do - expect(group.max_member_access_for_user(admin, only_concrete_membership: true)) - .to eq(Gitlab::Access::NO_ACCESS) - end - end - - context 'when max_access_for_group is set' do - let(:max_member_access) { 111 } - - before do - group_user.max_access_for_group[group.id] = max_member_access - end - - it 'uses the cached value' do - expect(group.max_member_access_for_user(group_user)).to eq(max_member_access) + expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) end end end - describe '#max_member_access' do - context 'group shared with another group' do - let_it_be(:parent_group_user) { create(:user) } - let_it_be(:child_group_user) { create(:user) } + context 'multiple groups shared with group' do + let(:user) { create(:user) } + let(:group) { create(:group, :private) } + let(:shared_group_parent) { create(:group, :private) } + let(:shared_group) { create(:group, :private, parent: shared_group_parent) } - let_it_be(:group_parent) { create(:group, :private) } - let_it_be(:group) { create(:group, :private, parent: group_parent) } - let_it_be(:group_child) { create(:group, :private, parent: group) } + before do + group.add_owner(user) - let_it_be(:shared_group_parent) { create(:group, :private) } - let_it_be(:shared_group) { create(:group, :private, parent: shared_group_parent) } - let_it_be(:shared_group_child) { create(:group, :private, parent: shared_group) } - - before do - group_parent.add_owner(parent_group_user) - group.add_owner(group_user) - group_child.add_owner(child_group_user) - - create(:group_group_link, { shared_with_group: group, - shared_group: shared_group, - group_access: GroupMember::DEVELOPER }) - end - - context 'with user in the group' do - it 'returns correct access level' do - expect(shared_group_parent.max_member_access(group_user)).to eq(Gitlab::Access::NO_ACCESS) - expect(shared_group.max_member_access(group_user)).to eq(Gitlab::Access::DEVELOPER) - expect(shared_group_child.max_member_access(group_user)).to eq(Gitlab::Access::DEVELOPER) - end - - context 'with lower group access level than max access level for share' do - let(:user) { create(:user) } - - it 'returns correct access level' do - group.add_reporter(user) - - expect(shared_group_parent.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS) - expect(shared_group.max_member_access(user)).to eq(Gitlab::Access::REPORTER) - expect(shared_group_child.max_member_access(user)).to eq(Gitlab::Access::REPORTER) - end - end - end - - context 'with user in the parent group' do - it 'returns correct access level' do - expect(shared_group_parent.max_member_access(parent_group_user)).to eq(Gitlab::Access::NO_ACCESS) - expect(shared_group.max_member_access(parent_group_user)).to eq(Gitlab::Access::NO_ACCESS) - expect(shared_group_child.max_member_access(parent_group_user)).to eq(Gitlab::Access::NO_ACCESS) - end - end - - context 'with user in the child group' do - it 'returns correct access level' do - expect(shared_group_parent.max_member_access(child_group_user)).to eq(Gitlab::Access::NO_ACCESS) - expect(shared_group.max_member_access(child_group_user)).to eq(Gitlab::Access::NO_ACCESS) - expect(shared_group_child.max_member_access(child_group_user)).to eq(Gitlab::Access::NO_ACCESS) - end - end - - context 'unrelated project owner' do - let(:common_id) { [Project.maximum(:id).to_i, Namespace.maximum(:id).to_i].max + 999 } - let!(:group) { create(:group, id: common_id) } - let!(:unrelated_project) { create(:project, id: common_id) } - let(:user) { unrelated_project.owner } - - it 'returns correct access level' do - expect(shared_group_parent.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS) - expect(shared_group.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS) - expect(shared_group_child.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS) - end - end - - context 'user without accepted access request' do - let!(:user) { create(:user) } - - before do - create(:group_member, :developer, :access_request, user: user, group: group) - end - - it 'returns correct access level' do - expect(shared_group_parent.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS) - expect(shared_group.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS) - expect(shared_group_child.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS) - end - end + create(:group_group_link, { shared_with_group: group, + shared_group: shared_group, + group_access: GroupMember::DEVELOPER }) + create(:group_group_link, { shared_with_group: group, + shared_group: shared_group_parent, + group_access: GroupMember::MAINTAINER }) end - context 'multiple groups shared with group' do - let(:user) { create(:user) } - let(:group) { create(:group, :private) } - let(:shared_group_parent) { create(:group, :private) } - let(:shared_group) { create(:group, :private, parent: shared_group_parent) } - - before do - group.add_owner(user) - - create(:group_group_link, { shared_with_group: group, - shared_group: shared_group, - group_access: GroupMember::DEVELOPER }) - create(:group_group_link, { shared_with_group: group, - shared_group: shared_group_parent, - group_access: GroupMember::MAINTAINER }) - end - - it 'returns correct access level' do - expect(shared_group.max_member_access(user)).to eq(Gitlab::Access::MAINTAINER) - end + it 'returns correct access level' do + expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::MAINTAINER) end end end