From b02a9e2d6f5000d85385c52aa239153eb9171cca Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Thu, 11 Apr 2024 08:19:07 +0200 Subject: [PATCH 1/7] Group visibility levels: Show all radio button with correct attributes - Shows all visibility levels with radio buttons - Checks only the radio button with the current visibility level - Disables disallowed and restricted radio buttons in order to make them not selectable Changelog: fixed --- app/helpers/visibility_level_helper.rb | 9 + app/views/shared/_visibility_radios.html.haml | 37 +++-- locale/gitlab.pot | 9 + .../general_visibility_levels_spec.rb | 155 ++++++++++++++++++ spec/helpers/visibility_level_helper_spec.rb | 35 ++++ 5 files changed, 232 insertions(+), 13 deletions(-) create mode 100644 spec/features/groups/settings/general_visibility_levels_spec.rb diff --git a/app/helpers/visibility_level_helper.rb b/app/helpers/visibility_level_helper.rb index 80c651f99ff2d7..d7a2316fc0d8e4 100644 --- a/app/helpers/visibility_level_helper.rb +++ b/app/helpers/visibility_level_helper.rb @@ -69,6 +69,10 @@ def selected_visibility_level(form_model, requested_level) [requested_level, max_allowed_visibility_level(form_model)].min end + def all_visibility_levels + Gitlab::VisibilityLevel.values + end + def available_visibility_levels(form_model) Gitlab::VisibilityLevel.values.reject do |level| disallowed_visibility_level?(form_model, level) || @@ -76,6 +80,11 @@ def available_visibility_levels(form_model) end end + def disabled_visibilty_level?(form_model, level) + disallowed_visibility_level?(form_model, level) || + restricted_visibility_levels.include?(level) + end + def snippets_selected_visibility_level(visibility_levels, selected) visibility_levels.find { |level| level == selected } || visibility_levels.min end diff --git a/app/views/shared/_visibility_radios.html.haml b/app/views/shared/_visibility_radios.html.haml index b4013cb5b80981..995d810c4fd19c 100644 --- a/app/views/shared/_visibility_radios.html.haml +++ b/app/views/shared/_visibility_radios.html.haml @@ -1,17 +1,28 @@ -- available_visibility_levels = available_visibility_levels(form_model) -- selected_level = snippets_selected_visibility_level(available_visibility_levels, selected_level) +- all_visibility_levels.each do |level| + - disabled_visibilty_level_icon_with_popover = capture do + - if disabled_visibilty_level?(form_model, level) + - popover_content = capture do + - if restricted_visibility_levels.include?(level) + %p + = _('This visibility level is restricted by the administrator.') + - if disallowed_visibility_level?(form_model, level) + %p + = _('This visibility level is not allowed because there is a child group or project with a higher visibility level.') -- available_visibility_levels.each do |level| + %span{ + data: { + testid: 'visibility-level-not-allowed-popover', + container: 'body', + content: popover_content, + html: 'true', + title: _('Visibility level not allowed'), + toggle: 'popover', + triggers: 'hover' } + } + = sprite_icon('lock') = form.gitlab_ui_radio_component model_method, level, - "#{visibility_level_icon(level)} #{visibility_level_label(level)}".html_safe, - help_text: '%{visibility_level_description}'.html_safe % { visibility_level_description: visibility_level_description(level, form_model)}, - radio_options: { checked: (selected_level == level), data: { track_label: "blank_project", track_action: "activate_form_input", track_property: "#{model_method}_#{level}", track_value: "" } }, + "#{visibility_level_icon(level)} #{visibility_level_label(level)} #{disabled_visibilty_level_icon_with_popover}".html_safe, + help_text: '%{visibility_level_description}%{option_disabled_reason}'.html_safe % { visibility_level_description: visibility_level_description(level, form_model), option_disabled_reason: 'Not allowed by administrators' }, + radio_options: { checked: (selected_level == level), disabled: disabled_visibilty_level?(form_model, level), data: { track_label: "blank_project", track_action: "activate_form_input", track_property: "#{model_method}_#{level}", track_value: "" } }, label_options: { class: 'js-visibility-level-radio' } - - -.text-muted - - if all_visibility_levels_restricted? - = _('Visibility settings have been disabled by the administrator.') - - elsif multiple_visibility_levels_restricted? - = _('Other visibility settings have been disabled by the administrator.') diff --git a/locale/gitlab.pot b/locale/gitlab.pot index a6a10b3b1eaeb3..77d60f6cd62fbe 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -53890,6 +53890,12 @@ msgstr "" msgid "This value is not valid." msgstr "" +msgid "This visibility level is not allowed because there is a child group or project with a higher visibility level." +msgstr "" + +msgid "This visibility level is restricted by the administrator." +msgstr "" + msgid "This vulnerability was automatically resolved because its vulnerability type was disabled in this project or removed from GitLab's default ruleset. For details about SAST rule changes, see https://docs.gitlab.com/ee/user/application_security/sast/rules#important-rule-changes." msgstr "" @@ -57618,6 +57624,9 @@ msgstr "" msgid "Visibility level" msgstr "" +msgid "Visibility level not allowed" +msgstr "" + msgid "Visibility level:" msgstr "" diff --git a/spec/features/groups/settings/general_visibility_levels_spec.rb b/spec/features/groups/settings/general_visibility_levels_spec.rb new file mode 100644 index 00000000000000..0dc4adcf555356 --- /dev/null +++ b/spec/features/groups/settings/general_visibility_levels_spec.rb @@ -0,0 +1,155 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'General settings visibility levels', :js, feature_category: :groups_and_projects do + let_it_be(:parent_group) { create(:group, :public) } + let_it_be(:user) { create(:user).tap { |user| parent_group.add_owner(user) } } + + before do + sign_in(user) + end + + context 'without restricted visibility levels' do + it 'shows each visibility level in correct field state' do + visit edit_group_path(parent_group) + + expect(page).to have_content('Visibility level') + + expect(page).to have_field("Private", checked: false, disabled: false) + expect(page).to have_field("Internal", checked: false, disabled: false) + expect(page).to have_field("Public", checked: true, disabled: false) + + expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Private') + expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Internal') + expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Public') + end + end + + context 'with restricted visibility level public' do + before do + stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC]) + end + + it 'shows each visibility level in correct field state' do + visit edit_group_path(parent_group) + + expect(page).to have_field("Private", checked: false, disabled: false) + expect(page).to have_field("Internal", checked: false, disabled: false) + expect(page).to have_field("Public", checked: true, disabled: true) + + expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Private') + expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Internal') + expect_popover_for_disallowed_visibility_level( + visibility_level_label_text: 'Public', + popover_content: 'This visibility level is restricted by the administrator.' + ) + end + + context 'with private project in group' do + let_it_be(:project) { create(:project, :private, group: parent_group) } + + it 'shows each visibility level in correct field state' do + visit edit_group_path(parent_group) + + expect(page).to have_field("Private", checked: false, disabled: false) + expect(page).to have_field("Internal", checked: false, disabled: false) + expect(page).to have_field("Public", checked: true, disabled: true) + + expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Private') + expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Internal') + expect_popover_for_disallowed_visibility_level( + visibility_level_label_text: 'Public', + popover_content: 'This visibility level is restricted by the administrator.' + ) + end + end + + context 'with public project in group' do + let_it_be(:project) { create(:project, :public, group: parent_group) } + + it 'shows each visibility level in correct field state' do + visit edit_group_path(parent_group) + + expect(page).to have_field("Private", checked: false, disabled: true) + expect(page).to have_field("Internal", checked: false, disabled: true) + expect(page).to have_field("Public", checked: true, disabled: true) + + expect_popover_for_disallowed_visibility_level( + visibility_level_label_text: 'Private', + popover_content: 'This visibility level is not allowed ' \ + 'because there is a child group or project with a higher visibility level.' + ) + + expect_popover_for_disallowed_visibility_level( + visibility_level_label_text: 'Internal', + popover_content: 'This visibility level is not allowed ' \ + 'because there is a child group or project with a higher visibility level.' + ) + + expect_popover_for_disallowed_visibility_level( + visibility_level_label_text: 'Public', + popover_content: 'This visibility level is restricted by the administrator.' + ) + end + end + end + + context 'with multiple restricted visibility levels "Public" and "Private"' do + let_it_be(:project) { create(:project, :internal, group: parent_group) } + + before do + stub_application_setting( + restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC, Gitlab::VisibilityLevel::PRIVATE] + ) + end + + it 'shows each visibility level in correct field state' do + visit edit_group_path(parent_group) + + expect(page).to have_field("Private", checked: false, disabled: true) + expect(page).to have_field("Internal", checked: false, disabled: false) + expect(page).to have_field("Public", checked: true, disabled: true) + + expect_popover_for_disallowed_visibility_level( + visibility_level_label_text: 'Private', + popover_content: [ + 'This visibility level is not allowed ' \ + 'because there is a child group or project with a higher visibility level.', + 'This visibility level is restricted by the administrator.' + ] + ) + + expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Internal') + + expect_popover_for_disallowed_visibility_level( + visibility_level_label_text: 'Public', + popover_content: 'This visibility level is restricted by the administrator.' + ) + end + end + + def expect_popover_for_disallowed_visibility_level(visibility_level_label_text:, popover_content:) + # Checking that a popover content is not visible + Array.wrap(popover_content).each { |content| expect(page).not_to have_content(content) } + + within('label', text: visibility_level_label_text) do + find('[data-testid=visibility-level-not-allowed-popover]').hover + end + + page.within('.gl-popover') do + Array.wrap(popover_content).each { |content| expect(page).to have_content(content) } + end + + # Move cursor to another element to hide the popover + find('label', text: visibility_level_label_text).hover + end + + def expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text:) + within('label', text: visibility_level_label_text) do + expect(page).not_to have_selector('[data-testid=visibility-level-not-allowed-popover]') + end + + expect(page).not_to have_selector('.gl-popover') + end +end diff --git a/spec/helpers/visibility_level_helper_spec.rb b/spec/helpers/visibility_level_helper_spec.rb index c2027c35ab3a3e..8cdab72246f12c 100644 --- a/spec/helpers/visibility_level_helper_spec.rb +++ b/spec/helpers/visibility_level_helper_spec.rb @@ -323,4 +323,39 @@ it { is_expected.to eq(expected) } end end + + describe 'restricted_visibilty_level?' do + using RSpec::Parameterized::TableSyntax + + let(:user) { create(:user) } + let(:level_public) { Gitlab::VisibilityLevel::PUBLIC } + let(:level_internal) { Gitlab::VisibilityLevel::INTERNAL } + let(:level_private) { Gitlab::VisibilityLevel::PRIVATE } + + subject { helper.restricted_visibilty_level?(visibility_level) } + + where(:restricted_visibility_levels, :visibility_level, :expected) do + [] | ref(:level_public) | false + [] | ref(:level_internal) | false + [] | ref(:level_private) | false + [ref(:level_public)] | ref(:level_public) | true + [ref(:level_public)] | ref(:level_internal) | false + [ref(:level_public)] | ref(:level_private) | false + [ref(:level_public), ref(:level_internal)] | ref(:level_public) | true + [ref(:level_public), ref(:level_internal)] | ref(:level_internal) | true + [ref(:level_public), ref(:level_internal)] | ref(:level_private) | false + Gitlab::VisibilityLevel.values | ref(:level_public) | true + Gitlab::VisibilityLevel.values | ref(:level_internal) | true + Gitlab::VisibilityLevel.values | ref(:level_private) | true + end + + with_them do + before do + allow(helper).to receive(:current_user) { user } + stub_application_setting(restricted_visibility_levels: restricted_visibility_levels) + end + + it { is_expected.to eq(expected) } + end + end end -- GitLab From 02f6c379f0e7c5ffdd4166ffc7303fa6a0a5dec4 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Tue, 23 Apr 2024 18:25:18 +0200 Subject: [PATCH 2/7] refactor: Apply suggestion from @lohrc, @lciutacu - Reduce scope of the MR to only show the icon and popover for restricted visibility levels - Disregard the disallowed visibility levels for now --- app/helpers/visibility_level_helper.rb | 9 +++- app/views/shared/_visibility_radios.html.haml | 22 +++------ locale/gitlab.pot | 6 --- .../general_visibility_levels_spec.rb | 46 +++++-------------- 4 files changed, 26 insertions(+), 57 deletions(-) diff --git a/app/helpers/visibility_level_helper.rb b/app/helpers/visibility_level_helper.rb index d7a2316fc0d8e4..fae9cb13968bb9 100644 --- a/app/helpers/visibility_level_helper.rb +++ b/app/helpers/visibility_level_helper.rb @@ -73,6 +73,12 @@ def all_visibility_levels Gitlab::VisibilityLevel.values end + def available_visibility_levels_without_restricted(form_model) + Gitlab::VisibilityLevel.values.reject do |level| + disallowed_visibility_level?(form_model, level) + end + end + def available_visibility_levels(form_model) Gitlab::VisibilityLevel.values.reject do |level| disallowed_visibility_level?(form_model, level) || @@ -80,8 +86,7 @@ def available_visibility_levels(form_model) end end - def disabled_visibilty_level?(form_model, level) - disallowed_visibility_level?(form_model, level) || + def restricted_visibilty_level?(level) restricted_visibility_levels.include?(level) end diff --git a/app/views/shared/_visibility_radios.html.haml b/app/views/shared/_visibility_radios.html.haml index 995d810c4fd19c..008263a0f2cc4a 100644 --- a/app/views/shared/_visibility_radios.html.haml +++ b/app/views/shared/_visibility_radios.html.haml @@ -1,19 +1,11 @@ -- all_visibility_levels.each do |level| - - disabled_visibilty_level_icon_with_popover = capture do - - if disabled_visibilty_level?(form_model, level) - - popover_content = capture do - - if restricted_visibility_levels.include?(level) - %p - = _('This visibility level is restricted by the administrator.') - - if disallowed_visibility_level?(form_model, level) - %p - = _('This visibility level is not allowed because there is a child group or project with a higher visibility level.') - +- available_visibility_levels_without_restricted(form_model).each do |level| + - restricted_visibilty_level_icon_with_popover = capture do + - if restricted_visibilty_level?(level) %span{ data: { - testid: 'visibility-level-not-allowed-popover', + testid: 'visibility-level-restricted-popover', container: 'body', - content: popover_content, + content: 'This visibility level has been restricted by your administrator.', html: 'true', title: _('Visibility level not allowed'), toggle: 'popover', @@ -22,7 +14,7 @@ = sprite_icon('lock') = form.gitlab_ui_radio_component model_method, level, - "#{visibility_level_icon(level)} #{visibility_level_label(level)} #{disabled_visibilty_level_icon_with_popover}".html_safe, + "#{visibility_level_icon(level)} #{visibility_level_label(level)} #{restricted_visibilty_level_icon_with_popover}".html_safe, help_text: '%{visibility_level_description}%{option_disabled_reason}'.html_safe % { visibility_level_description: visibility_level_description(level, form_model), option_disabled_reason: 'Not allowed by administrators' }, - radio_options: { checked: (selected_level == level), disabled: disabled_visibilty_level?(form_model, level), data: { track_label: "blank_project", track_action: "activate_form_input", track_property: "#{model_method}_#{level}", track_value: "" } }, + radio_options: { checked: (selected_level == level), disabled: restricted_visibilty_level?(level), data: { track_label: "blank_project", track_action: "activate_form_input", track_property: "#{model_method}_#{level}", track_value: "" } }, label_options: { class: 'js-visibility-level-radio' } diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 77d60f6cd62fbe..d63232360e47ec 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -53890,12 +53890,6 @@ msgstr "" msgid "This value is not valid." msgstr "" -msgid "This visibility level is not allowed because there is a child group or project with a higher visibility level." -msgstr "" - -msgid "This visibility level is restricted by the administrator." -msgstr "" - msgid "This vulnerability was automatically resolved because its vulnerability type was disabled in this project or removed from GitLab's default ruleset. For details about SAST rule changes, see https://docs.gitlab.com/ee/user/application_security/sast/rules#important-rule-changes." msgstr "" diff --git a/spec/features/groups/settings/general_visibility_levels_spec.rb b/spec/features/groups/settings/general_visibility_levels_spec.rb index 0dc4adcf555356..cb0de126457667 100644 --- a/spec/features/groups/settings/general_visibility_levels_spec.rb +++ b/spec/features/groups/settings/general_visibility_levels_spec.rb @@ -34,7 +34,7 @@ it 'shows each visibility level in correct field state' do visit edit_group_path(parent_group) - expect(page).to have_field("Private", checked: false, disabled: false) + expect(page).to have_field("Private", checked: false, disabled: false) expect(page).to have_field("Internal", checked: false, disabled: false) expect(page).to have_field("Public", checked: true, disabled: true) @@ -42,7 +42,7 @@ expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Internal') expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Public', - popover_content: 'This visibility level is restricted by the administrator.' + popover_content: 'This visibility level has been restricted by your administrator.' ) end @@ -52,7 +52,7 @@ it 'shows each visibility level in correct field state' do visit edit_group_path(parent_group) - expect(page).to have_field("Private", checked: false, disabled: false) + expect(page).to have_field("Private", checked: false, disabled: false) expect(page).to have_field("Internal", checked: false, disabled: false) expect(page).to have_field("Public", checked: true, disabled: true) @@ -60,7 +60,7 @@ expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Internal') expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Public', - popover_content: 'This visibility level is restricted by the administrator.' + popover_content: 'This visibility level has been restricted by your administrator.' ) end end @@ -71,25 +71,13 @@ it 'shows each visibility level in correct field state' do visit edit_group_path(parent_group) - expect(page).to have_field("Private", checked: false, disabled: true) - expect(page).to have_field("Internal", checked: false, disabled: true) - expect(page).to have_field("Public", checked: true, disabled: true) - - expect_popover_for_disallowed_visibility_level( - visibility_level_label_text: 'Private', - popover_content: 'This visibility level is not allowed ' \ - 'because there is a child group or project with a higher visibility level.' - ) - - expect_popover_for_disallowed_visibility_level( - visibility_level_label_text: 'Internal', - popover_content: 'This visibility level is not allowed ' \ - 'because there is a child group or project with a higher visibility level.' - ) + expect(page).not_to have_field("Private") + expect(page).not_to have_field("Internal") + expect(page).to have_field("Public", checked: true, disabled: true) expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Public', - popover_content: 'This visibility level is restricted by the administrator.' + popover_content: 'This visibility level has been restricted by your administrator.' ) end end @@ -107,24 +95,14 @@ it 'shows each visibility level in correct field state' do visit edit_group_path(parent_group) - expect(page).to have_field("Private", checked: false, disabled: true) + expect(page).not_to have_field("Private") expect(page).to have_field("Internal", checked: false, disabled: false) expect(page).to have_field("Public", checked: true, disabled: true) - expect_popover_for_disallowed_visibility_level( - visibility_level_label_text: 'Private', - popover_content: [ - 'This visibility level is not allowed ' \ - 'because there is a child group or project with a higher visibility level.', - 'This visibility level is restricted by the administrator.' - ] - ) - expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Internal') - expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Public', - popover_content: 'This visibility level is restricted by the administrator.' + popover_content: 'This visibility level has been restricted by your administrator.' ) end end @@ -134,7 +112,7 @@ def expect_popover_for_disallowed_visibility_level(visibility_level_label_text:, Array.wrap(popover_content).each { |content| expect(page).not_to have_content(content) } within('label', text: visibility_level_label_text) do - find('[data-testid=visibility-level-not-allowed-popover]').hover + find('[data-testid=visibility-level-restricted-popover]').hover end page.within('.gl-popover') do @@ -147,7 +125,7 @@ def expect_popover_for_disallowed_visibility_level(visibility_level_label_text:, def expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text:) within('label', text: visibility_level_label_text) do - expect(page).not_to have_selector('[data-testid=visibility-level-not-allowed-popover]') + expect(page).not_to have_selector('[data-testid=visibility-level-restricted-popover]') end expect(page).not_to have_selector('.gl-popover') -- GitLab From a1979f5d0ec5fc4b980cda0af671fba9b8f65e74 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Tue, 30 Apr 2024 17:57:08 +0200 Subject: [PATCH 3/7] refactor: Apply suggestion from @lohrc, @lciutacu - Revert changes from previous commit and extend the logic for the popover content - Popover for disabled visibility level radio button is shown when: - Visibility level is disabled because a child item (project or subgroup) has less restrictive visibility level - Visibility level is disabled because the parent group has a restrictive visibility level --- app/helpers/visibility_level_helper.rb | 29 ++++- app/views/shared/_visibility_radios.html.haml | 27 +++-- locale/gitlab.pot | 9 ++ .../general_visibility_levels_spec.rb | 111 ++++++++++++++---- spec/helpers/visibility_level_helper_spec.rb | 71 ++++++++++- 5 files changed, 211 insertions(+), 36 deletions(-) diff --git a/app/helpers/visibility_level_helper.rb b/app/helpers/visibility_level_helper.rb index fae9cb13968bb9..bbe2ae43e37cc6 100644 --- a/app/helpers/visibility_level_helper.rb +++ b/app/helpers/visibility_level_helper.rb @@ -53,6 +53,24 @@ def disallowed_visibility_level?(form_model, level) !form_model.visibility_level_allowed?(level) end + def disallowed_visibility_level_by_parent?(form_model, level) + return false unless form_model.respond_to?(:visibility_level_allowed_by_parent?) + + !form_model.visibility_level_allowed_by_parent?(level) + end + + def disallowed_visibility_level_by_projects?(form_model, level) + return false unless form_model.respond_to?(:visibility_level_allowed_by_projects?) + + !form_model.visibility_level_allowed_by_projects?(level) + end + + def disallowed_visibility_level_by_sub_groups?(form_model, level) + return false unless form_model.respond_to?(:visibility_level_allowed_by_sub_groups?) + + !form_model.visibility_level_allowed_by_sub_groups?(level) + end + # Visibility level can be restricted in two ways: # # 1. The group permissions (e.g. a subgroup is private, which requires @@ -73,12 +91,6 @@ def all_visibility_levels Gitlab::VisibilityLevel.values end - def available_visibility_levels_without_restricted(form_model) - Gitlab::VisibilityLevel.values.reject do |level| - disallowed_visibility_level?(form_model, level) - end - end - def available_visibility_levels(form_model) Gitlab::VisibilityLevel.values.reject do |level| disallowed_visibility_level?(form_model, level) || @@ -90,6 +102,11 @@ def restricted_visibilty_level?(level) restricted_visibility_levels.include?(level) end + def disabled_visibilty_level?(form_model, level) + disallowed_visibility_level?(form_model, level) || + restricted_visibilty_level?(level) + end + def snippets_selected_visibility_level(visibility_levels, selected) visibility_levels.find { |level| level == selected } || visibility_levels.min end diff --git a/app/views/shared/_visibility_radios.html.haml b/app/views/shared/_visibility_radios.html.haml index 008263a0f2cc4a..fd22a574eab0fa 100644 --- a/app/views/shared/_visibility_radios.html.haml +++ b/app/views/shared/_visibility_radios.html.haml @@ -1,11 +1,24 @@ -- available_visibility_levels_without_restricted(form_model).each do |level| - - restricted_visibilty_level_icon_with_popover = capture do - - if restricted_visibilty_level?(level) +- all_visibility_levels.each do |level| + - disabled_visibilty_level_icon_with_popover = capture do + - if disabled_visibilty_level?(form_model, level) + - popover_content = capture do + - if restricted_visibility_levels.include?(level) + %p + = s_('VisibilityLevel|This visibility level is restricted by the administrator.') + - if disallowed_visibility_level_by_parent?(form_model, level) + %p + = s_('VisibilityLevel|This visibility level is not allowed because the parent group has a more restrictive visibility level.') + - if disallowed_visibility_level_by_projects?(form_model, level) || disallowed_visibility_level_by_sub_groups?(form_model, level) + %p + - learn_more_link_start = ''.html_safe # rubocop:disable Gitlab/DocUrl -- Not referencing this rails application; it is referencing another doc + - learn_more_link_end = ''.html_safe + = s_('VisibilityLevel|This visibility level is not allowed because a child of %{group_name} has a less restrictive visibility level. %{learn_more_link_start}Learn more%{learn_more_link_end}.').html_safe % { group_name: form_model.name, learn_more_link_start: learn_more_link_start, learn_more_link_end: learn_more_link_end } + %span{ data: { - testid: 'visibility-level-restricted-popover', + testid: 'visibility-level-not-allowed-popover', container: 'body', - content: 'This visibility level has been restricted by your administrator.', + content: popover_content, html: 'true', title: _('Visibility level not allowed'), toggle: 'popover', @@ -14,7 +27,7 @@ = sprite_icon('lock') = form.gitlab_ui_radio_component model_method, level, - "#{visibility_level_icon(level)} #{visibility_level_label(level)} #{restricted_visibilty_level_icon_with_popover}".html_safe, + "#{visibility_level_icon(level)} #{visibility_level_label(level)} #{disabled_visibilty_level_icon_with_popover}".html_safe, help_text: '%{visibility_level_description}%{option_disabled_reason}'.html_safe % { visibility_level_description: visibility_level_description(level, form_model), option_disabled_reason: 'Not allowed by administrators' }, - radio_options: { checked: (selected_level == level), disabled: restricted_visibilty_level?(level), data: { track_label: "blank_project", track_action: "activate_form_input", track_property: "#{model_method}_#{level}", track_value: "" } }, + radio_options: { checked: (selected_level == level), disabled: disabled_visibilty_level?(form_model, level), data: { track_label: "blank_project", track_action: "activate_form_input", track_property: "#{model_method}_#{level}", track_value: "" } }, label_options: { class: 'js-visibility-level-radio' } diff --git a/locale/gitlab.pot b/locale/gitlab.pot index d63232360e47ec..f5349c2c7e3222 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -57663,6 +57663,15 @@ msgstr "" msgid "VisibilityLevel|The project can be accessed without any authentication." msgstr "" +msgid "VisibilityLevel|This visibility level is not allowed because a child of %{group_name} has a less restrictive visibility level. %{learn_more_link_start}Learn more%{learn_more_link_end}." +msgstr "" + +msgid "VisibilityLevel|This visibility level is not allowed because the parent group has a more restrictive visibility level." +msgstr "" + +msgid "VisibilityLevel|This visibility level is restricted by the administrator." +msgstr "" + msgid "VisibilityLevel|Unknown" msgstr "" diff --git a/spec/features/groups/settings/general_visibility_levels_spec.rb b/spec/features/groups/settings/general_visibility_levels_spec.rb index cb0de126457667..45a153e67310d3 100644 --- a/spec/features/groups/settings/general_visibility_levels_spec.rb +++ b/spec/features/groups/settings/general_visibility_levels_spec.rb @@ -3,16 +3,61 @@ require 'spec_helper' RSpec.describe 'General settings visibility levels', :js, feature_category: :groups_and_projects do - let_it_be(:parent_group) { create(:group, :public) } - let_it_be(:user) { create(:user).tap { |user| parent_group.add_owner(user) } } + let_it_be(:group) { create(:group, :public) } + let_it_be(:user) { create(:user).tap { |user| group.add_owner(user) } } before do sign_in(user) end + context 'with parent group internal' do + let_it_be(:parent_group) { create(:group, :internal) } + let_it_be(:group) { create(:group, :internal, parent: parent_group) } + let_it_be(:user) { create(:user).tap { |user| group.add_owner(user) } } + + it 'shows each visibility level in correct field state' do + visit edit_group_path(group) + + expect(page).to have_content('Visibility level') + + expect(page).to have_field("Private", checked: false, disabled: false) + expect(page).to have_field("Internal", checked: true, disabled: false) + expect(page).to have_field("Public", checked: false, disabled: true) + + expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Private') + expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Internal') + expect_popover_for_disallowed_visibility_level( + visibility_level_label_text: 'Public', + popover_content: + 'This visibility level is not allowed because the parent group has a more restrictive visibility level.' + ) + end + end + + context 'with internal child project in group' do + let_it_be(:project) { create(:project, :internal, group: group) } + + it 'shows each visibility level in correct field state' do + visit edit_group_path(group) + + expect(page).to have_field("Private", checked: false, disabled: true) + expect(page).to have_field("Internal", checked: false, disabled: false) + expect(page).to have_field("Public", checked: true, disabled: false) + + expect_popover_for_disallowed_visibility_level( + visibility_level_label_text: 'Private', + popover_content: + 'This visibility level is not allowed ' \ + "because a child of #{group.name} has a less restrictive visibility level. Learn more." + ) + expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Internal') + expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Public') + end + end + context 'without restricted visibility levels' do it 'shows each visibility level in correct field state' do - visit edit_group_path(parent_group) + visit edit_group_path(group) expect(page).to have_content('Visibility level') @@ -32,9 +77,9 @@ end it 'shows each visibility level in correct field state' do - visit edit_group_path(parent_group) + visit edit_group_path(group) - expect(page).to have_field("Private", checked: false, disabled: false) + expect(page).to have_field("Private", checked: false, disabled: false) expect(page).to have_field("Internal", checked: false, disabled: false) expect(page).to have_field("Public", checked: true, disabled: true) @@ -42,17 +87,17 @@ expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Internal') expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Public', - popover_content: 'This visibility level has been restricted by your administrator.' + popover_content: 'This visibility level is restricted by the administrator.' ) end context 'with private project in group' do - let_it_be(:project) { create(:project, :private, group: parent_group) } + let_it_be(:project) { create(:project, :private, group: group) } it 'shows each visibility level in correct field state' do - visit edit_group_path(parent_group) + visit edit_group_path(group) - expect(page).to have_field("Private", checked: false, disabled: false) + expect(page).to have_field("Private", checked: false, disabled: false) expect(page).to have_field("Internal", checked: false, disabled: false) expect(page).to have_field("Public", checked: true, disabled: true) @@ -60,31 +105,43 @@ expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Internal') expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Public', - popover_content: 'This visibility level has been restricted by your administrator.' + popover_content: 'This visibility level is restricted by the administrator.' ) end end context 'with public project in group' do - let_it_be(:project) { create(:project, :public, group: parent_group) } + let_it_be(:project) { create(:project, :public, group: group) } it 'shows each visibility level in correct field state' do - visit edit_group_path(parent_group) + visit edit_group_path(group) + + expect(page).to have_field("Private", checked: false, disabled: true) + expect(page).to have_field("Internal", checked: false, disabled: true) + expect(page).to have_field("Public", checked: true, disabled: true) - expect(page).not_to have_field("Private") - expect(page).not_to have_field("Internal") - expect(page).to have_field("Public", checked: true, disabled: true) + expect_popover_for_disallowed_visibility_level( + visibility_level_label_text: 'Private', + popover_content: 'This visibility level is not allowed ' \ + "because a child of #{group.name} has a less restrictive visibility level. Learn more." + ) + + expect_popover_for_disallowed_visibility_level( + visibility_level_label_text: 'Internal', + popover_content: 'This visibility level is not allowed ' \ + "because a child of #{group.name} has a less restrictive visibility level. Learn more." + ) expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Public', - popover_content: 'This visibility level has been restricted by your administrator.' + popover_content: 'This visibility level is restricted by the administrator.' ) end end end context 'with multiple restricted visibility levels "Public" and "Private"' do - let_it_be(:project) { create(:project, :internal, group: parent_group) } + let_it_be(:project) { create(:project, :internal, group: group) } before do stub_application_setting( @@ -93,16 +150,26 @@ end it 'shows each visibility level in correct field state' do - visit edit_group_path(parent_group) + visit edit_group_path(group) - expect(page).not_to have_field("Private") + expect(page).to have_field("Private", checked: false, disabled: true) expect(page).to have_field("Internal", checked: false, disabled: false) expect(page).to have_field("Public", checked: true, disabled: true) + expect_popover_for_disallowed_visibility_level( + visibility_level_label_text: 'Private', + popover_content: [ + 'This visibility level is not allowed ' \ + "because a child of #{group.name} has a less restrictive visibility level. Learn more.", + 'This visibility level is restricted by the administrator.' + ] + ) + expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Internal') + expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Public', - popover_content: 'This visibility level has been restricted by your administrator.' + popover_content: 'This visibility level is restricted by the administrator.' ) end end @@ -112,7 +179,7 @@ def expect_popover_for_disallowed_visibility_level(visibility_level_label_text:, Array.wrap(popover_content).each { |content| expect(page).not_to have_content(content) } within('label', text: visibility_level_label_text) do - find('[data-testid=visibility-level-restricted-popover]').hover + find('[data-testid=visibility-level-not-allowed-popover]').hover end page.within('.gl-popover') do @@ -125,7 +192,7 @@ def expect_popover_for_disallowed_visibility_level(visibility_level_label_text:, def expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text:) within('label', text: visibility_level_label_text) do - expect(page).not_to have_selector('[data-testid=visibility-level-restricted-popover]') + expect(page).not_to have_selector('[data-testid=visibility-level-not-allowed-popover]') end expect(page).not_to have_selector('.gl-popover') diff --git a/spec/helpers/visibility_level_helper_spec.rb b/spec/helpers/visibility_level_helper_spec.rb index 8cdab72246f12c..cdb1f8ecd8fb8f 100644 --- a/spec/helpers/visibility_level_helper_spec.rb +++ b/spec/helpers/visibility_level_helper_spec.rb @@ -355,7 +355,76 @@ stub_application_setting(restricted_visibility_levels: restricted_visibility_levels) end - it { is_expected.to eq(expected) } + it { is_expected.to eq expected } + end + end + + describe 'disallowed_visibility_level_by_parent?' do + using RSpec::Parameterized::TableSyntax + + let(:parent_group) { create(:group, parent_group_visibility_level) } + let(:group) { build(:group, :private, parent: parent_group) } + + subject { helper.disallowed_visibility_level_by_parent?(group, visibility_level) } + + where(:parent_group_visibility_level, :visibility_level, :expected) do + :public | Gitlab::VisibilityLevel::PUBLIC | false + :public | Gitlab::VisibilityLevel::INTERNAL | false + :public | Gitlab::VisibilityLevel::PRIVATE | false + :internal | Gitlab::VisibilityLevel::PUBLIC | true + :internal | Gitlab::VisibilityLevel::INTERNAL | false + :internal | Gitlab::VisibilityLevel::PRIVATE | false + :private | Gitlab::VisibilityLevel::PUBLIC | true + :private | Gitlab::VisibilityLevel::INTERNAL | true + :private | Gitlab::VisibilityLevel::PRIVATE | false + end + + with_them do + it { is_expected.to eq expected } + end + end + + describe 'disallowed_visibility_level_by_projects? and disallowed_visibility_level_by_sub_groups?' do + using RSpec::Parameterized::TableSyntax + + let_it_be(:group) { create(:group, :public) } + + where(:child_visibility_level, :visibility_level, :expected) do + Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::PUBLIC | false + Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::INTERNAL | true + Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::PRIVATE | true + Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::PUBLIC | false + Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::INTERNAL | false + Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::PRIVATE | true + Gitlab::VisibilityLevel::PRIVATE | Gitlab::VisibilityLevel::PUBLIC | false + Gitlab::VisibilityLevel::PRIVATE | Gitlab::VisibilityLevel::INTERNAL | false + Gitlab::VisibilityLevel::PRIVATE | Gitlab::VisibilityLevel::PRIVATE | false + end + + with_them do + describe 'disallowed_visibility_level_by_projects?' do + let_it_be(:subproject) { create(:project, group: group) } + + before do + subproject.update!(visibility_level: child_visibility_level) + end + + subject { helper.disallowed_visibility_level_by_projects?(group, visibility_level) } + + it { is_expected.to eq expected } + end + + describe 'disallowed_visibility_level_by_sub_groups?' do + let_it_be(:subgroup) { create(:group, parent: group) } + + before do + subgroup.update!(visibility_level: child_visibility_level) + end + + subject { helper.disallowed_visibility_level_by_sub_groups?(group, visibility_level) } + + it { is_expected.to eq expected } + end end end end -- GitLab From 7681e2a7439fd740b271b7959176636bb69a884d Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Thu, 2 May 2024 10:14:23 +0200 Subject: [PATCH 4/7] refactor: Apply suggestion from @lohrc, @mnichols1 - Reference / show only one restriction in cases where multiple restrictions apply; - This way, we do not overwhelm or confuse the user about what action to take in line with the design principle "Progressive disclosure", see https://design.gitlab.com/usability/progressive-disclosure --- app/views/shared/_visibility_radios.html.haml | 17 +++++------ locale/gitlab.pot | 6 ++-- .../general_visibility_levels_spec.rb | 28 ++++++++----------- 3 files changed, 22 insertions(+), 29 deletions(-) diff --git a/app/views/shared/_visibility_radios.html.haml b/app/views/shared/_visibility_radios.html.haml index fd22a574eab0fa..f823bbf5c2342e 100644 --- a/app/views/shared/_visibility_radios.html.haml +++ b/app/views/shared/_visibility_radios.html.haml @@ -3,16 +3,13 @@ - if disabled_visibilty_level?(form_model, level) - popover_content = capture do - if restricted_visibility_levels.include?(level) - %p - = s_('VisibilityLevel|This visibility level is restricted by the administrator.') - - if disallowed_visibility_level_by_parent?(form_model, level) - %p - = s_('VisibilityLevel|This visibility level is not allowed because the parent group has a more restrictive visibility level.') - - if disallowed_visibility_level_by_projects?(form_model, level) || disallowed_visibility_level_by_sub_groups?(form_model, level) - %p - - learn_more_link_start = ''.html_safe # rubocop:disable Gitlab/DocUrl -- Not referencing this rails application; it is referencing another doc - - learn_more_link_end = ''.html_safe - = s_('VisibilityLevel|This visibility level is not allowed because a child of %{group_name} has a less restrictive visibility level. %{learn_more_link_start}Learn more%{learn_more_link_end}.').html_safe % { group_name: form_model.name, learn_more_link_start: learn_more_link_start, learn_more_link_end: learn_more_link_end } + = s_('VisibilityLevel|This visibility level has been restricted by your administrator.') + - elsif disallowed_visibility_level_by_parent?(form_model, level) + = s_('VisibilityLevel|This visibility level is not allowed because the parent group has a more restrictive visibility level.') + - elsif disallowed_visibility_level_by_projects?(form_model, level) || disallowed_visibility_level_by_sub_groups?(form_model, level) + - learn_more_link_start = ''.html_safe # rubocop:disable Gitlab/DocUrl -- Not referencing this rails application; it is referencing another doc + - learn_more_link_end = ''.html_safe + = s_('VisibilityLevel|This visibility level is not allowed because a child of %{group_name} has a less restrictive visibility level. %{learn_more_link_start}Learn more%{learn_more_link_end}.').html_safe % { group_name: form_model.name, learn_more_link_start: learn_more_link_start, learn_more_link_end: learn_more_link_end } %span{ data: { diff --git a/locale/gitlab.pot b/locale/gitlab.pot index f5349c2c7e3222..91f59b521f8fe8 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -57663,13 +57663,13 @@ msgstr "" msgid "VisibilityLevel|The project can be accessed without any authentication." msgstr "" -msgid "VisibilityLevel|This visibility level is not allowed because a child of %{group_name} has a less restrictive visibility level. %{learn_more_link_start}Learn more%{learn_more_link_end}." +msgid "VisibilityLevel|This visibility level has been restricted by your administrator." msgstr "" -msgid "VisibilityLevel|This visibility level is not allowed because the parent group has a more restrictive visibility level." +msgid "VisibilityLevel|This visibility level is not allowed because a child of %{group_name} has a less restrictive visibility level. %{learn_more_link_start}Learn more%{learn_more_link_end}." msgstr "" -msgid "VisibilityLevel|This visibility level is restricted by the administrator." +msgid "VisibilityLevel|This visibility level is not allowed because the parent group has a more restrictive visibility level." msgstr "" msgid "VisibilityLevel|Unknown" diff --git a/spec/features/groups/settings/general_visibility_levels_spec.rb b/spec/features/groups/settings/general_visibility_levels_spec.rb index 45a153e67310d3..8b84720047bce1 100644 --- a/spec/features/groups/settings/general_visibility_levels_spec.rb +++ b/spec/features/groups/settings/general_visibility_levels_spec.rb @@ -28,8 +28,8 @@ expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Internal') expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Public', - popover_content: - 'This visibility level is not allowed because the parent group has a more restrictive visibility level.' + popover_content: 'This visibility level is not allowed ' \ + 'because the parent group has a more restrictive visibility level.' ) end end @@ -46,9 +46,8 @@ expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Private', - popover_content: - 'This visibility level is not allowed ' \ - "because a child of #{group.name} has a less restrictive visibility level. Learn more." + popover_content: "This visibility level is not allowed " \ + "because a child of #{group.name} has a less restrictive visibility level. Learn more." ) expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Internal') expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Public') @@ -87,7 +86,7 @@ expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Internal') expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Public', - popover_content: 'This visibility level is restricted by the administrator.' + popover_content: 'This visibility level has been restricted by your administrator.' ) end @@ -105,7 +104,7 @@ expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Internal') expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Public', - popover_content: 'This visibility level is restricted by the administrator.' + popover_content: 'This visibility level has been restricted by your administrator.' ) end end @@ -122,19 +121,19 @@ expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Private', - popover_content: 'This visibility level is not allowed ' \ + popover_content: "This visibility level is not allowed " \ "because a child of #{group.name} has a less restrictive visibility level. Learn more." ) expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Internal', - popover_content: 'This visibility level is not allowed ' \ + popover_content: "This visibility level is not allowed " \ "because a child of #{group.name} has a less restrictive visibility level. Learn more." ) expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Public', - popover_content: 'This visibility level is restricted by the administrator.' + popover_content: 'This visibility level has been restricted by your administrator.' ) end end @@ -158,18 +157,14 @@ expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Private', - popover_content: [ - 'This visibility level is not allowed ' \ - "because a child of #{group.name} has a less restrictive visibility level. Learn more.", - 'This visibility level is restricted by the administrator.' - ] + popover_content: 'This visibility level has been restricted by your administrator.' ) expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Internal') expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Public', - popover_content: 'This visibility level is restricted by the administrator.' + popover_content: 'This visibility level has been restricted by your administrator.' ) end end @@ -183,6 +178,7 @@ def expect_popover_for_disallowed_visibility_level(visibility_level_label_text:, end page.within('.gl-popover') do + expect(page).to have_content('Visibility level not allowed') Array.wrap(popover_content).each { |content| expect(page).to have_content(content) } end -- GitLab From 4011470460823c4edc3cef97a6e5fa59454206f4 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Wed, 8 May 2024 13:09:32 +0000 Subject: [PATCH 5/7] refactor: Apply suggestion from @dmeshcharakou - Add missing tests for methods in `VisibilityLevelHelper` --- app/helpers/visibility_level_helper.rb | 8 +- app/views/shared/_visibility_radios.html.haml | 2 +- .../general_visibility_levels_spec.rb | 8 +- spec/helpers/visibility_level_helper_spec.rb | 212 +++++++++++------- 4 files changed, 145 insertions(+), 85 deletions(-) diff --git a/app/helpers/visibility_level_helper.rb b/app/helpers/visibility_level_helper.rb index bbe2ae43e37cc6..81b64a6c10ef6b 100644 --- a/app/helpers/visibility_level_helper.rb +++ b/app/helpers/visibility_level_helper.rb @@ -98,15 +98,15 @@ def available_visibility_levels(form_model) end end - def restricted_visibilty_level?(level) - restricted_visibility_levels.include?(level) - end - def disabled_visibilty_level?(form_model, level) disallowed_visibility_level?(form_model, level) || restricted_visibilty_level?(level) end + def restricted_visibilty_level?(level) + restricted_visibility_levels.include?(level) + end + def snippets_selected_visibility_level(visibility_levels, selected) visibility_levels.find { |level| level == selected } || visibility_levels.min end diff --git a/app/views/shared/_visibility_radios.html.haml b/app/views/shared/_visibility_radios.html.haml index f823bbf5c2342e..459e5858765e19 100644 --- a/app/views/shared/_visibility_radios.html.haml +++ b/app/views/shared/_visibility_radios.html.haml @@ -2,7 +2,7 @@ - disabled_visibilty_level_icon_with_popover = capture do - if disabled_visibilty_level?(form_model, level) - popover_content = capture do - - if restricted_visibility_levels.include?(level) + - if restricted_visibilty_level?(level) = s_('VisibilityLevel|This visibility level has been restricted by your administrator.') - elsif disallowed_visibility_level_by_parent?(form_model, level) = s_('VisibilityLevel|This visibility level is not allowed because the parent group has a more restrictive visibility level.') diff --git a/spec/features/groups/settings/general_visibility_levels_spec.rb b/spec/features/groups/settings/general_visibility_levels_spec.rb index 8b84720047bce1..5ac63f5374e5ff 100644 --- a/spec/features/groups/settings/general_visibility_levels_spec.rb +++ b/spec/features/groups/settings/general_visibility_levels_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe 'General settings visibility levels', :js, feature_category: :groups_and_projects do +RSpec.describe 'General settings visibility levels', :js, :aggregate_failures, feature_category: :groups_and_projects do let_it_be(:group) { create(:group, :public) } let_it_be(:user) { create(:user).tap { |user| group.add_owner(user) } } @@ -170,8 +170,8 @@ end def expect_popover_for_disallowed_visibility_level(visibility_level_label_text:, popover_content:) - # Checking that a popover content is not visible - Array.wrap(popover_content).each { |content| expect(page).not_to have_content(content) } + # Checking that a popover content is not visible before hovering + expect(page).not_to have_content(popover_content) within('label', text: visibility_level_label_text) do find('[data-testid=visibility-level-not-allowed-popover]').hover @@ -179,7 +179,7 @@ def expect_popover_for_disallowed_visibility_level(visibility_level_label_text:, page.within('.gl-popover') do expect(page).to have_content('Visibility level not allowed') - Array.wrap(popover_content).each { |content| expect(page).to have_content(content) } + expect(page).to have_content(popover_content) end # Move cursor to another element to hide the popover diff --git a/spec/helpers/visibility_level_helper_spec.rb b/spec/helpers/visibility_level_helper_spec.rb index cdb1f8ecd8fb8f..37b5e4c2d8901c 100644 --- a/spec/helpers/visibility_level_helper_spec.rb +++ b/spec/helpers/visibility_level_helper_spec.rb @@ -152,6 +152,75 @@ end end + describe '#disallowed_visibility_level_by_parent?' do + using RSpec::Parameterized::TableSyntax + + let(:parent_group) { create(:group, parent_group_visibility_level) } + let(:group) { build(:group, :private, parent: parent_group) } + + subject { helper.disallowed_visibility_level_by_parent?(group, visibility_level) } + + where(:parent_group_visibility_level, :visibility_level, :expected) do + :public | Gitlab::VisibilityLevel::PUBLIC | false + :public | Gitlab::VisibilityLevel::INTERNAL | false + :public | Gitlab::VisibilityLevel::PRIVATE | false + :internal | Gitlab::VisibilityLevel::PUBLIC | true + :internal | Gitlab::VisibilityLevel::INTERNAL | false + :internal | Gitlab::VisibilityLevel::PRIVATE | false + :private | Gitlab::VisibilityLevel::PUBLIC | true + :private | Gitlab::VisibilityLevel::INTERNAL | true + :private | Gitlab::VisibilityLevel::PRIVATE | false + end + + with_them do + it { is_expected.to eq expected } + end + end + + shared_examples_for 'disallowed visibility level by child' do + using RSpec::Parameterized::TableSyntax + + where(:child_visibility_level, :visibility_level, :expected) do + Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::PUBLIC | false + Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::INTERNAL | true + Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::PRIVATE | true + Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::PUBLIC | false + Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::INTERNAL | false + Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::PRIVATE | true + Gitlab::VisibilityLevel::PRIVATE | Gitlab::VisibilityLevel::PUBLIC | false + Gitlab::VisibilityLevel::PRIVATE | Gitlab::VisibilityLevel::INTERNAL | false + Gitlab::VisibilityLevel::PRIVATE | Gitlab::VisibilityLevel::PRIVATE | false + end + + with_them do + before do + child.update!(visibility_level: child_visibility_level) + end + + it { is_expected.to eq expected } + end + end + + describe '#disallowed_visibility_level_by_projects?' do + let_it_be(:group) { create(:group, :public) } + + let_it_be_with_reload(:child) { create(:project, group: group) } + + subject { helper.disallowed_visibility_level_by_projects?(group, visibility_level) } + + it_behaves_like 'disallowed visibility level by child' + end + + describe '#disallowed_visibility_level_by_sub_groups?' do + let_it_be(:group) { create(:group, :public) } + + let_it_be_with_reload(:child) { create(:group, parent: group) } + + subject { helper.disallowed_visibility_level_by_sub_groups?(group, visibility_level) } + + it_behaves_like 'disallowed visibility level by child' + end + describe "selected_visibility_level" do let(:group) { create(:group, :public) } let!(:project) { create(:project, :internal, group: group) } @@ -324,107 +393,98 @@ end end - describe 'restricted_visibilty_level?' do + describe '#all_visibility_levels' do + subject { helper.all_visibility_levels } + + it 'returns all visibility levels' do + is_expected.to eq [ + Gitlab::VisibilityLevel::PRIVATE, + Gitlab::VisibilityLevel::INTERNAL, + Gitlab::VisibilityLevel::PUBLIC + ] + end + end + + describe '#disabled_visibilty_level?' do using RSpec::Parameterized::TableSyntax - let(:user) { create(:user) } - let(:level_public) { Gitlab::VisibilityLevel::PUBLIC } - let(:level_internal) { Gitlab::VisibilityLevel::INTERNAL } - let(:level_private) { Gitlab::VisibilityLevel::PRIVATE } + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group, :public) } + let_it_be_with_reload(:child) { create(:project, group: group) } - subject { helper.restricted_visibilty_level?(visibility_level) } + subject { helper.disabled_visibilty_level?(group, visibility_level) } - where(:restricted_visibility_levels, :visibility_level, :expected) do - [] | ref(:level_public) | false - [] | ref(:level_internal) | false - [] | ref(:level_private) | false - [ref(:level_public)] | ref(:level_public) | true - [ref(:level_public)] | ref(:level_internal) | false - [ref(:level_public)] | ref(:level_private) | false - [ref(:level_public), ref(:level_internal)] | ref(:level_public) | true - [ref(:level_public), ref(:level_internal)] | ref(:level_internal) | true - [ref(:level_public), ref(:level_internal)] | ref(:level_private) | false - Gitlab::VisibilityLevel.values | ref(:level_public) | true - Gitlab::VisibilityLevel.values | ref(:level_internal) | true - Gitlab::VisibilityLevel.values | ref(:level_private) | true + where(:restricted_visibility_levels, :child_visibility_level, :visibility_level, :expected) do + [] | Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::PUBLIC | false + [] | Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::INTERNAL | true + [] | Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::PRIVATE | true + [] | Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::PUBLIC | false + [] | Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::INTERNAL | false + [] | Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::PRIVATE | true + + [Gitlab::VisibilityLevel::PUBLIC] | Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::PUBLIC | true + [Gitlab::VisibilityLevel::PUBLIC] | Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::INTERNAL | true + [Gitlab::VisibilityLevel::PUBLIC] | Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::PRIVATE | true + [Gitlab::VisibilityLevel::PUBLIC] | Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::PUBLIC | true + [Gitlab::VisibilityLevel::PUBLIC] | Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::INTERNAL | false + [Gitlab::VisibilityLevel::PUBLIC] | Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::PRIVATE | true + + [Gitlab::VisibilityLevel::INTERNAL] | Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::PUBLIC | false + [Gitlab::VisibilityLevel::INTERNAL] | Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::INTERNAL | true + [Gitlab::VisibilityLevel::INTERNAL] | Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::PRIVATE | true + [Gitlab::VisibilityLevel::INTERNAL] | Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::PUBLIC | false + [Gitlab::VisibilityLevel::INTERNAL] | Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::INTERNAL | true + [Gitlab::VisibilityLevel::INTERNAL] | Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::PRIVATE | true + + [Gitlab::VisibilityLevel::PUBLIC, Gitlab::VisibilityLevel::INTERNAL] | Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::PUBLIC | true + [Gitlab::VisibilityLevel::PUBLIC, Gitlab::VisibilityLevel::INTERNAL] | Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::INTERNAL | true + [Gitlab::VisibilityLevel::PUBLIC, Gitlab::VisibilityLevel::INTERNAL] | Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::INTERNAL | true + + Gitlab::VisibilityLevel.values | Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::PUBLIC | true + Gitlab::VisibilityLevel.values | Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::INTERNAL | true end with_them do before do allow(helper).to receive(:current_user) { user } stub_application_setting(restricted_visibility_levels: restricted_visibility_levels) + + child.update!(visibility_level: child_visibility_level) end it { is_expected.to eq expected } end end - describe 'disallowed_visibility_level_by_parent?' do + describe '#restricted_visibilty_level?' do using RSpec::Parameterized::TableSyntax - let(:parent_group) { create(:group, parent_group_visibility_level) } - let(:group) { build(:group, :private, parent: parent_group) } - - subject { helper.disallowed_visibility_level_by_parent?(group, visibility_level) } - - where(:parent_group_visibility_level, :visibility_level, :expected) do - :public | Gitlab::VisibilityLevel::PUBLIC | false - :public | Gitlab::VisibilityLevel::INTERNAL | false - :public | Gitlab::VisibilityLevel::PRIVATE | false - :internal | Gitlab::VisibilityLevel::PUBLIC | true - :internal | Gitlab::VisibilityLevel::INTERNAL | false - :internal | Gitlab::VisibilityLevel::PRIVATE | false - :private | Gitlab::VisibilityLevel::PUBLIC | true - :private | Gitlab::VisibilityLevel::INTERNAL | true - :private | Gitlab::VisibilityLevel::PRIVATE | false - end - - with_them do - it { is_expected.to eq expected } - end - end - - describe 'disallowed_visibility_level_by_projects? and disallowed_visibility_level_by_sub_groups?' do - using RSpec::Parameterized::TableSyntax + let(:user) { create(:user) } - let_it_be(:group) { create(:group, :public) } + subject { helper.restricted_visibilty_level?(visibility_level) } - where(:child_visibility_level, :visibility_level, :expected) do - Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::PUBLIC | false - Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::INTERNAL | true - Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::PRIVATE | true - Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::PUBLIC | false - Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::INTERNAL | false - Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::PRIVATE | true - Gitlab::VisibilityLevel::PRIVATE | Gitlab::VisibilityLevel::PUBLIC | false - Gitlab::VisibilityLevel::PRIVATE | Gitlab::VisibilityLevel::INTERNAL | false - Gitlab::VisibilityLevel::PRIVATE | Gitlab::VisibilityLevel::PRIVATE | false + where(:restricted_visibility_levels, :visibility_level, :expected) do + [] | Gitlab::VisibilityLevel::PUBLIC | false + [] | Gitlab::VisibilityLevel::INTERNAL | false + [] | Gitlab::VisibilityLevel::PRIVATE | false + [Gitlab::VisibilityLevel::PUBLIC] | Gitlab::VisibilityLevel::PUBLIC | true + [Gitlab::VisibilityLevel::PUBLIC] | Gitlab::VisibilityLevel::INTERNAL | false + [Gitlab::VisibilityLevel::PUBLIC] | Gitlab::VisibilityLevel::PRIVATE | false + [Gitlab::VisibilityLevel::PUBLIC, Gitlab::VisibilityLevel::INTERNAL] | Gitlab::VisibilityLevel::PUBLIC | true + [Gitlab::VisibilityLevel::PUBLIC, Gitlab::VisibilityLevel::INTERNAL] | Gitlab::VisibilityLevel::INTERNAL | true + [Gitlab::VisibilityLevel::PUBLIC, Gitlab::VisibilityLevel::INTERNAL] | Gitlab::VisibilityLevel::PRIVATE | false + Gitlab::VisibilityLevel.values | Gitlab::VisibilityLevel::PUBLIC | true + Gitlab::VisibilityLevel.values | Gitlab::VisibilityLevel::INTERNAL | true + Gitlab::VisibilityLevel.values | Gitlab::VisibilityLevel::PRIVATE | true end with_them do - describe 'disallowed_visibility_level_by_projects?' do - let_it_be(:subproject) { create(:project, group: group) } - - before do - subproject.update!(visibility_level: child_visibility_level) - end - - subject { helper.disallowed_visibility_level_by_projects?(group, visibility_level) } - - it { is_expected.to eq expected } + before do + allow(helper).to receive(:current_user) { user } + stub_application_setting(restricted_visibility_levels: restricted_visibility_levels) end - describe 'disallowed_visibility_level_by_sub_groups?' do - let_it_be(:subgroup) { create(:group, parent: group) } - - before do - subgroup.update!(visibility_level: child_visibility_level) - end - - subject { helper.disallowed_visibility_level_by_sub_groups?(group, visibility_level) } - - it { is_expected.to eq expected } - end + it { is_expected.to eq expected } end end end -- GitLab From b7822701ef353485f9007351aa070408d16de782 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Mon, 13 May 2024 12:30:13 +0000 Subject: [PATCH 6/7] refactor: Apply suggestion from @dmeshcharakou - Fix typo in method name - Add more test cases --- app/helpers/visibility_level_helper.rb | 6 +++--- app/views/shared/_visibility_radios.html.haml | 10 +++++----- .../groups/settings/general_visibility_levels_spec.rb | 8 ++++---- spec/helpers/visibility_level_helper_spec.rb | 11 +++++++---- 4 files changed, 19 insertions(+), 16 deletions(-) diff --git a/app/helpers/visibility_level_helper.rb b/app/helpers/visibility_level_helper.rb index 81b64a6c10ef6b..9795b367c5356a 100644 --- a/app/helpers/visibility_level_helper.rb +++ b/app/helpers/visibility_level_helper.rb @@ -98,12 +98,12 @@ def available_visibility_levels(form_model) end end - def disabled_visibilty_level?(form_model, level) + def disabled_visibility_level?(form_model, level) disallowed_visibility_level?(form_model, level) || - restricted_visibilty_level?(level) + restricted_visibility_level?(level) end - def restricted_visibilty_level?(level) + def restricted_visibility_level?(level) restricted_visibility_levels.include?(level) end diff --git a/app/views/shared/_visibility_radios.html.haml b/app/views/shared/_visibility_radios.html.haml index 459e5858765e19..5f1352f63723a3 100644 --- a/app/views/shared/_visibility_radios.html.haml +++ b/app/views/shared/_visibility_radios.html.haml @@ -1,8 +1,8 @@ - all_visibility_levels.each do |level| - - disabled_visibilty_level_icon_with_popover = capture do - - if disabled_visibilty_level?(form_model, level) + - disabled_visibility_level_icon_with_popover = capture do + - if disabled_visibility_level?(form_model, level) - popover_content = capture do - - if restricted_visibilty_level?(level) + - if restricted_visibility_level?(level) = s_('VisibilityLevel|This visibility level has been restricted by your administrator.') - elsif disallowed_visibility_level_by_parent?(form_model, level) = s_('VisibilityLevel|This visibility level is not allowed because the parent group has a more restrictive visibility level.') @@ -24,7 +24,7 @@ = sprite_icon('lock') = form.gitlab_ui_radio_component model_method, level, - "#{visibility_level_icon(level)} #{visibility_level_label(level)} #{disabled_visibilty_level_icon_with_popover}".html_safe, + "#{visibility_level_icon(level)} #{visibility_level_label(level)} #{disabled_visibility_level_icon_with_popover}".html_safe, help_text: '%{visibility_level_description}%{option_disabled_reason}'.html_safe % { visibility_level_description: visibility_level_description(level, form_model), option_disabled_reason: 'Not allowed by administrators' }, - radio_options: { checked: (selected_level == level), disabled: disabled_visibilty_level?(form_model, level), data: { track_label: "blank_project", track_action: "activate_form_input", track_property: "#{model_method}_#{level}", track_value: "" } }, + radio_options: { checked: (selected_level == level), disabled: disabled_visibility_level?(form_model, level), data: { track_label: "blank_project", track_action: "activate_form_input", track_property: "#{model_method}_#{level}", track_value: "" } }, label_options: { class: 'js-visibility-level-radio' } diff --git a/spec/features/groups/settings/general_visibility_levels_spec.rb b/spec/features/groups/settings/general_visibility_levels_spec.rb index 5ac63f5374e5ff..b3eead86ffcca1 100644 --- a/spec/features/groups/settings/general_visibility_levels_spec.rb +++ b/spec/features/groups/settings/general_visibility_levels_spec.rb @@ -29,7 +29,7 @@ expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Public', popover_content: 'This visibility level is not allowed ' \ - 'because the parent group has a more restrictive visibility level.' + 'because the parent group has a more restrictive visibility level.' ) end end @@ -47,7 +47,7 @@ expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Private', popover_content: "This visibility level is not allowed " \ - "because a child of #{group.name} has a less restrictive visibility level. Learn more." + "because a child of #{group.name} has a less restrictive visibility level. Learn more." ) expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Internal') expect_no_popover_for_disallowed_visibility_level(visibility_level_label_text: 'Public') @@ -122,13 +122,13 @@ expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Private', popover_content: "This visibility level is not allowed " \ - "because a child of #{group.name} has a less restrictive visibility level. Learn more." + "because a child of #{group.name} has a less restrictive visibility level. Learn more." ) expect_popover_for_disallowed_visibility_level( visibility_level_label_text: 'Internal', popover_content: "This visibility level is not allowed " \ - "because a child of #{group.name} has a less restrictive visibility level. Learn more." + "because a child of #{group.name} has a less restrictive visibility level. Learn more." ) expect_popover_for_disallowed_visibility_level( diff --git a/spec/helpers/visibility_level_helper_spec.rb b/spec/helpers/visibility_level_helper_spec.rb index 37b5e4c2d8901c..7a90a79d56e095 100644 --- a/spec/helpers/visibility_level_helper_spec.rb +++ b/spec/helpers/visibility_level_helper_spec.rb @@ -405,14 +405,14 @@ end end - describe '#disabled_visibilty_level?' do + describe '#disabled_visibility_level?' do using RSpec::Parameterized::TableSyntax let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group, :public) } let_it_be_with_reload(:child) { create(:project, group: group) } - subject { helper.disabled_visibilty_level?(group, visibility_level) } + subject { helper.disabled_visibility_level?(group, visibility_level) } where(:restricted_visibility_levels, :child_visibility_level, :visibility_level, :expected) do [] | Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::PUBLIC | false @@ -421,6 +421,9 @@ [] | Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::PUBLIC | false [] | Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::INTERNAL | false [] | Gitlab::VisibilityLevel::INTERNAL | Gitlab::VisibilityLevel::PRIVATE | true + [] | Gitlab::VisibilityLevel::PRIVATE | Gitlab::VisibilityLevel::PUBLIC | false + [] | Gitlab::VisibilityLevel::PRIVATE | Gitlab::VisibilityLevel::INTERNAL | false + [] | Gitlab::VisibilityLevel::PRIVATE | Gitlab::VisibilityLevel::PRIVATE | false [Gitlab::VisibilityLevel::PUBLIC] | Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::PUBLIC | true [Gitlab::VisibilityLevel::PUBLIC] | Gitlab::VisibilityLevel::PUBLIC | Gitlab::VisibilityLevel::INTERNAL | true @@ -456,12 +459,12 @@ end end - describe '#restricted_visibilty_level?' do + describe '#restricted_visibility_level?' do using RSpec::Parameterized::TableSyntax let(:user) { create(:user) } - subject { helper.restricted_visibilty_level?(visibility_level) } + subject { helper.restricted_visibility_level?(visibility_level) } where(:restricted_visibility_levels, :visibility_level, :expected) do [] | Gitlab::VisibilityLevel::PUBLIC | false -- GitLab From 2e2ca43f74bd25129b1be70f10cb793528bc71ba Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Fri, 24 May 2024 09:48:48 +0200 Subject: [PATCH 7/7] test: Fix failing tests and ci pipeline --- spec/features/projects/new_project_spec.rb | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/spec/features/projects/new_project_spec.rb b/spec/features/projects/new_project_spec.rb index d6b27d8c618faf..e78b2469601762 100644 --- a/spec/features/projects/new_project_spec.rb +++ b/spec/features/projects/new_project_spec.rb @@ -69,7 +69,7 @@ end end - it 'shows a message if multiple levels are restricted' do + it 'disables the radio button for visibility levels "Private" and "Internal"' do stub_application_setting(default_project_visibility: Gitlab::VisibilityLevel::PUBLIC) stub_application_setting( restricted_visibility_levels: [Gitlab::VisibilityLevel::PRIVATE, Gitlab::VisibilityLevel::INTERNAL] @@ -78,16 +78,20 @@ visit new_project_path click_link 'Create blank project' - expect(page).to have_content 'Other visibility settings have been disabled by the administrator.' + expect(page).to have_field("Private", checked: false, disabled: true) + expect(page).to have_field("Internal", checked: false, disabled: true) + expect(page).to have_field("Public", checked: true, disabled: false) end - it 'shows a message if all levels are restricted' do + it 'disables all radio button for visibility levels' do stub_application_setting(restricted_visibility_levels: Gitlab::VisibilityLevel.values) visit new_project_path click_link 'Create blank project' - expect(page).to have_content 'Visibility settings have been disabled by the administrator.' + expect(page).to have_field("Private", checked: true, disabled: true) + expect(page).to have_field("Internal", checked: false, disabled: true) + expect(page).to have_field("Public", checked: false, disabled: true) end end -- GitLab