Merge branch...

Merge branch '350347-enabling-features-in-project-visibility-settings-can-lead-to-incorrect-you-are-about-to' into 'master'

Fix incorrect confirmation modal prompt when updating project settings

See merge request gitlab-org/gitlab!78353

app/assets/javascripts/pages/projects/shared/permissions/components/settings_panel.vue

 <script>
-import { GlIcon, GlSprintf, GlLink, GlFormCheckbox, GlToggle } from '@gitlab/ui';
-
+import { GlButton, GlIcon, GlSprintf, GlLink, GlFormCheckbox, GlToggle } from '@gitlab/ui';
+import ConfirmDanger from '~/vue_shared/components/confirm_danger/confirm_danger.vue';
 import settingsMixin from 'ee_else_ce/pages/projects/shared/permissions/mixins/settings_pannel_mixin';
 import { __, s__ } from '~/locale';
 import {
@@ -41,16 +41,19 @@ export default {
     pucWarningHelpText: s__(
       'ProjectSettings|Highlight the usage of hidden unicode characters. These have innocent uses for right-to-left languages, but can also be used in potential exploits.',
     ),
+    confirmButtonText: __('Save changes'),
   },
 
   components: {
     projectFeatureSetting,
     projectSettingRow,
+    GlButton,
     GlIcon,
     GlSprintf,
     GlLink,
     GlFormCheckbox,
     GlToggle,
+    ConfirmDanger,
   },
   mixins: [settingsMixin],
 
@@ -163,6 +166,15 @@ export default {
       required: false,
       default: '',
     },
+    confirmationPhrase: {
+      type: String,
+      required: true,
+    },
+    showVisibilityConfirmModal: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
   },
   data() {
     const defaults = {
@@ -274,6 +286,12 @@ export default {
     cveIdRequestIsDisabled() {
       return this.visibilityLevel !== visibilityOptions.PUBLIC;
     },
+    isVisibilityReduced() {
+      return (
+        this.showVisibilityConfirmModal &&
+        this.visibilityLevel < this.currentSettings.visibilityLevel
+      );
+    },
   },
 
   watch: {
@@ -774,5 +792,24 @@ export default {
         <template #help>{{ $options.i18n.pucWarningHelpText }}</template>
       </gl-form-checkbox>
     </project-setting-row>
+    <confirm-danger
+      v-if="isVisibilityReduced"
+      button-class="qa-visibility-features-permissions-save-button"
+      button-variant="confirm"
+      :disabled="false"
+      :phrase="confirmationPhrase"
+      :button-text="$options.i18n.confirmButtonText"
+      data-testid="project-features-save-button"
+      @confirm="$emit('confirm')"
+    />
+    <gl-button
+      v-else
+      type="submit"
+      variant="confirm"
+      data-testid="project-features-save-button"
+      button-class="qa-visibility-features-permissions-save-button"
+    >
+      {{ $options.i18n.confirmButtonText }}
+    </gl-button>
   </div>
 </template>

app/assets/javascripts/pages/projects/shared/permissions/index.js

 import Vue from 'vue';
+import { parseBoolean } from '~/lib/utils/common_utils';
 import settingsPanel from './components/settings_panel.vue';
 
 export default function initProjectPermissionsSettings() {
@@ -6,8 +7,36 @@ export default function initProjectPermissionsSettings() {
   const componentPropsEl = document.querySelector('.js-project-permissions-form-data');
   const componentProps = JSON.parse(componentPropsEl.innerHTML);
 
+  const {
+    targetFormId,
+    additionalInformation,
+    confirmDangerMessage,
+    confirmButtonText,
+    showVisibilityConfirmModal,
+    htmlConfirmationMessage,
+    phrase: confirmationPhrase,
+  } = mountPoint.dataset;
+
   return new Vue({
     el: mountPoint,
-    render: (createElement) => createElement(settingsPanel, { props: { ...componentProps } }),
+    provide: {
+      additionalInformation,
+      confirmDangerMessage,
+      confirmButtonText,
+      htmlConfirmationMessage: parseBoolean(htmlConfirmationMessage),
+    },
+    render: (createElement) =>
+      createElement(settingsPanel, {
+        props: {
+          ...componentProps,
+          confirmationPhrase,
+          showVisibilityConfirmModal: parseBoolean(showVisibilityConfirmModal),
+        },
+        on: {
+          confirm: () => {
+            if (targetFormId) document.getElementById(targetFormId)?.submit();
+          },
+        },
+      }),
   });
 }

app/helpers/projects_helper.rb

@@ -672,18 +672,16 @@ module ProjectsHelper
     html_escape(message) % { strong_start: strong_start, strong_end: strong_end, project_name: project.name, group_name: project.group ? project.group.name : nil }
   end
 
-  def visibility_confirm_modal_data(project, remove_form_id = nil)
+  def visibility_confirm_modal_data(project, target_form_id = nil)
     {
-      remove_form_id: remove_form_id,
-      qa_selector: 'visibility_features_permissions_save_button',
-      button_text: _('Save changes'),
+      target_form_id: target_form_id,
       button_testid: 'reduce-project-visibility-button',
-      button_variant: 'confirm',
       confirm_button_text: _('Reduce project visibility'),
       confirm_danger_message: confirm_reduce_visibility_message(project),
       phrase: project.full_path,
       additional_information: _('Note: current forks will keep their visibility level.'),
-      html_confirmation_message: true
+      html_confirmation_message: true.to_s,
+      show_visibility_confirm_modal: show_visibility_confirm_modal?(project).to_s
     }
   end
 

app/views/projects/edit.html.haml

@@ -2,7 +2,7 @@
 - page_title _("General")
 - @content_class = "limit-container-width" unless fluid_layout
 - expanded = expanded_by_default?
-- remove_visibility_form_id = 'reduce-visibility-form'
+- reduce_visibility_form_id = 'reduce-visibility-form'
 
 %section.settings.general-settings.no-animate.expanded#js-general-settings
   .settings-header
@@ -18,11 +18,10 @@
     %p= _('Choose visibility level, enable/disable project features and their permissions, disable email notifications, and show default award emoji.')
 
   .settings-content
-    = form_for @project, html: { multipart: true, class: "sharing-permissions-form", id: remove_visibility_form_id }, authenticity_token: true do |f|
+    = form_for @project, html: { multipart: true, class: "sharing-permissions-form", id: reduce_visibility_form_id }, authenticity_token: true do |f|
       %input{ name: 'update_section', type: 'hidden', value: 'js-shared-permissions' }
       %template.js-project-permissions-form-data{ type: "application/json" }= project_permissions_panel_data(@project).to_json.html_safe
-      .js-project-permissions-form
-      = f.submit _('Save changes'), class: "btn gl-button btn-confirm #{('js-confirm-danger' if show_visibility_confirm_modal?(@project))}", data: visibility_confirm_modal_data(@project, remove_visibility_form_id)
+      .js-project-permissions-form{ data: visibility_confirm_modal_data(@project, reduce_visibility_form_id) }
 
 %section.rspec-merge-request-settings.settings.merge-requests-feature.no-animate#js-merge-request-settings{ class: [('expanded' if expanded), ('hidden' if @project.project_feature.send(:merge_requests_access_level) == 0)], data: { qa_selector: 'merge_request_settings_content' } }
   .settings-header

qa/qa/page/project/settings/visibility_features_permissions.rb

@@ -5,12 +5,9 @@ module QA
     module Project
       module Settings
         class VisibilityFeaturesPermissions < Page::Base
-          view 'app/helpers/projects_helper.rb' do
-            element :visibility_features_permissions_save_button
-          end
-
           view 'app/assets/javascripts/pages/projects/shared/permissions/components/settings_panel.vue' do
             element :project_visibility_dropdown
+            element :visibility_features_permissions_save_button
           end
 
           def set_project_visibility(visibility)

spec/features/projects/features_visibility_spec.rb

@@ -24,7 +24,7 @@ RSpec.describe 'Edit Project Settings' do
           # disable by clicking toggle
           toggle_feature_off("project[project_feature_attributes][#{tool_name}_access_level]")
           page.within('.sharing-permissions') do
-            find('input[value="Save changes"]').click
+            find('[data-testid="project-features-save-button"]').click
           end
           wait_for_requests
           expect(page).not_to have_selector(".shortcuts-#{shortcut_name}")
@@ -32,7 +32,7 @@ RSpec.describe 'Edit Project Settings' do
           # re-enable by clicking toggle again
           toggle_feature_on("project[project_feature_attributes][#{tool_name}_access_level]")
           page.within('.sharing-permissions') do
-            find('input[value="Save changes"]').click
+            find('[data-testid="project-features-save-button"]').click
           end
           wait_for_requests
           expect(page).to have_selector(".shortcuts-#{shortcut_name}")

spec/features/projects/settings/project_settings_spec.rb

@@ -47,7 +47,7 @@ RSpec.describe 'Projects settings' do
       # disable by clicking toggle
       forking_enabled_button.click
       page.within('.sharing-permissions') do
-        find('input[value="Save changes"]').click
+        find('[data-testid="project-features-save-button"]').click
       end
       wait_for_requests
 
@@ -77,7 +77,7 @@ RSpec.describe 'Projects settings' do
       expect(default_award_emojis_input.value).to eq('false')
 
       page.within('.sharing-permissions') do
-        find('input[value="Save changes"]').click
+        find('[data-testid="project-features-save-button"]').click
       end
       wait_for_requests
 

spec/features/projects/settings/user_manages_merge_requests_settings_spec.rb

@@ -54,7 +54,7 @@ RSpec.describe 'Projects > Settings > User manages merge request settings' do
 
         within('.sharing-permissions-form') do
           find('.project-feature-controls[data-for="project[project_feature_attributes][merge_requests_access_level]"] .gl-toggle').click
-          find('input[value="Save changes"]').send_keys(:return)
+          find('[data-testid="project-features-save-button"]').send_keys(:return)
         end
 
         expect(page).not_to have_content 'Pipelines must succeed'
@@ -74,7 +74,7 @@ RSpec.describe 'Projects > Settings > User manages merge request settings' do
 
         within('.sharing-permissions-form') do
           find('.project-feature-controls[data-for="project[project_feature_attributes][builds_access_level]"] .gl-toggle').click
-          find('input[value="Save changes"]').send_keys(:return)
+          find('[data-testid="project-features-save-button"]').send_keys(:return)
         end
 
         expect(page).to have_content 'Pipelines must succeed'
@@ -95,7 +95,7 @@ RSpec.describe 'Projects > Settings > User manages merge request settings' do
 
       within('.sharing-permissions-form') do
         find('.project-feature-controls[data-for="project[project_feature_attributes][merge_requests_access_level]"] .gl-toggle').click
-        find('input[value="Save changes"]').send_keys(:return)
+        find('[data-testid="project-features-save-button"]').send_keys(:return)
       end
 
       expect(page).to have_content 'Pipelines must succeed'

spec/features/projects/user_changes_project_visibility_spec.rb

@@ -5,14 +5,6 @@ require 'spec_helper'
 RSpec.describe 'User changes public project visibility', :js do
   include ProjectForksHelper
 
-  before do
-    fork_project(project, project.owner)
-
-    sign_in(project.owner)
-
-    visit edit_project_path(project)
-  end
-
   shared_examples 'changing visibility to private' do
     it 'requires confirmation' do
       visibility_select = first('.project-feature-controls .select-control')
@@ -34,15 +26,85 @@ RSpec.describe 'User changes public project visibility', :js do
     end
   end
 
-  context 'when a project is public' do
+  shared_examples 'does not require confirmation' do
+    it 'saves without confirmation' do
+      visibility_select = first('.project-feature-controls .select-control')
+      visibility_select.select('Private')
+
+      page.within('#js-shared-permissions') do
+        click_button 'Save changes'
+      end
+
+      wait_for_requests
+
+      expect(project.reload).to be_private
+    end
+  end
+
+  context 'when the project has forks' do
+    before do
+      fork_project(project, project.owner)
+
+      sign_in(project.owner)
+
+      visit edit_project_path(project)
+    end
+
+    context 'when a project is public' do
+      let(:project) { create(:project, :empty_repo, :public) }
+
+      it_behaves_like 'changing visibility to private'
+    end
+
+    context 'when the project is internal' do
+      let(:project) { create(:project, :empty_repo, :internal) }
+
+      it_behaves_like 'changing visibility to private'
+    end
+
+    context 'when the visibility level is untouched' do
+      let(:project) { create(:project, :empty_repo, :public) }
+
+      it 'saves without confirmation' do
+        expect(page).to have_selector('.js-emails-disabled', visible: true)
+        find('.js-emails-disabled input[type="checkbox"]').click
+
+        page.within('#js-shared-permissions') do
+          click_button 'Save changes'
+        end
+
+        wait_for_requests
+
+        expect(project.reload).to be_public
+      end
+    end
+  end
+
+  context 'when the project is not forked' do
     let(:project) { create(:project, :empty_repo, :public) }
 
-    it_behaves_like 'changing visibility to private'
+    before do
+      sign_in(project.owner)
+
+      visit edit_project_path(project)
+    end
+
+    it_behaves_like 'does not require confirmation'
   end
 
-  context 'when the project is internal' do
-    let(:project) { create(:project, :empty_repo, :internal) }
+  context 'with unlink_fork_network_upon_visibility_decrease = false' do
+    let(:project) { create(:project, :empty_repo, :public) }
+
+    before do
+      stub_feature_flags(unlink_fork_network_upon_visibility_decrease: false)
+
+      fork_project(project, project.owner)
+
+      sign_in(project.owner)
+
+      visit edit_project_path(project)
+    end
 
-    it_behaves_like 'changing visibility to private'
+    it_behaves_like 'does not require confirmation'
   end
 end

spec/frontend/pages/projects/shared/permissions/components/settings_panel_spec.js

@@ -7,6 +7,7 @@ import {
   visibilityLevelDescriptions,
   visibilityOptions,
 } from '~/pages/projects/shared/permissions/constants';
+import ConfirmDanger from '~/vue_shared/components/confirm_danger/confirm_danger.vue';
 
 const defaultProps = {
   currentSettings: {
@@ -47,6 +48,8 @@ const defaultProps = {
   packagesAvailable: false,
   packagesHelpPath: '/help/user/packages/index',
   requestCveAvailable: true,
+  confirmationPhrase: 'my-fake-project',
+  showVisibilityConfirmModal: false,
 };
 
 describe('Settings Panel', () => {
@@ -104,6 +107,7 @@ describe('Settings Panel', () => {
     );
   const findMetricsVisibilitySettings = () => wrapper.find({ ref: 'metrics-visibility-settings' });
   const findOperationsSettings = () => wrapper.find({ ref: 'operations-settings' });
+  const findConfirmDangerButton = () => wrapper.findComponent(ConfirmDanger);
 
   afterEach(() => {
     wrapper.destroy();
@@ -177,6 +181,44 @@ describe('Settings Panel', () => {
 
       expect(findRequestAccessEnabledInput().exists()).toBe(false);
     });
+
+    it('does not require confirmation if the visibility is reduced', async () => {
+      wrapper = mountComponent({
+        currentSettings: { visibilityLevel: visibilityOptions.INTERNAL },
+      });
+
+      expect(findConfirmDangerButton().exists()).toBe(false);
+
+      await findProjectVisibilityLevelInput().setValue(visibilityOptions.PRIVATE);
+
+      expect(findConfirmDangerButton().exists()).toBe(false);
+    });
+
+    describe('showVisibilityConfirmModal=true', () => {
+      beforeEach(() => {
+        wrapper = mountComponent({
+          currentSettings: { visibilityLevel: visibilityOptions.INTERNAL },
+          showVisibilityConfirmModal: true,
+        });
+      });
+
+      it('will render the confirmation dialog if the visibility is reduced', async () => {
+        expect(findConfirmDangerButton().exists()).toBe(false);
+
+        await findProjectVisibilityLevelInput().setValue(visibilityOptions.PRIVATE);
+
+        expect(findConfirmDangerButton().exists()).toBe(true);
+      });
+
+      it('emits the `confirm` event when the reduce visibility warning is confirmed', async () => {
+        expect(wrapper.emitted('confirm')).toBeUndefined();
+
+        await findProjectVisibilityLevelInput().setValue(visibilityOptions.PRIVATE);
+        await findConfirmDangerButton().vm.$emit('confirm');
+
+        expect(wrapper.emitted('confirm')).toHaveLength(1);
+      });
+    });
   });
 
   describe('Issues settings', () => {