Merge branch '328612-fix-verify-saml-configuration-button-in-saml-settings' into 'master'

Fix `Verify SAML Configuration` button

See merge request gitlab-org/gitlab!80978

app/assets/javascripts/dirty_submit/dirty_submit_form.js

 import $ from 'jquery';
 import { memoize, throttle } from 'lodash';
+import createEventHub from '~/helpers/event_hub_factory';
 
 class DirtySubmitForm {
   constructor(form) {
     this.form = form;
     this.dirtyInputs = [];
     this.isDisabled = true;
+    this.events = createEventHub();
 
     this.init();
   }
@@ -36,11 +38,21 @@ class DirtySubmitForm {
     this.form.addEventListener('submit', (event) => this.formSubmit(event));
   }
 
+  addInputsListener(callback) {
+    this.events.$on('input', callback);
+  }
+
+  removeInputsListener(callback) {
+    this.events.$off('input', callback);
+  }
+
   updateDirtyInput(event) {
     const { target } = event;
 
     if (!target.dataset.isDirtySubmitInput) return;
 
+    this.events.$emit('input', event);
+
     this.updateDirtyInputs(target);
     this.toggleSubmission();
   }

ee/app/assets/javascripts/saml_providers/saml_settings_form.js

@@ -21,9 +21,9 @@ function getCallout(el) {
 
 function toggleElementVisibility(el, show) {
   if (show) {
-    el.classList.remove('gl-display-none');
+    el?.classList.remove('gl-display-none');
   } else {
-    el.classList.add('gl-display-none');
+    el?.classList.add('gl-display-none');
   }
 }
 
@@ -66,8 +66,9 @@ export default class SamlSettingsForm {
 
     this.testButtonTooltipWrapper = this.form.querySelector('#js-saml-test-button');
     this.testButton = this.testButtonTooltipWrapper.querySelector('a');
+    this.testButtonDisabled = this.testButtonTooltipWrapper.querySelector('button[disabled]');
     this.dirtyFormChecker = dirtySubmitFactory(this.form);
-    this.form.addEventListener('change', this.handleChangeEvent);
+    this.dirtyFormChecker.addInputsListener(this.handleInputsChange);
   }
 
   findSetting(name) {
@@ -99,11 +100,12 @@ export default class SamlSettingsForm {
     this.updateView();
   }
 
-  handleChangeEvent = (event) => {
+  handleInputsChange = (event) => {
     if (this.settingIsDefined(event.target)) {
       this.updateSAMLSettings();
-      this.updateView();
     }
+
+    this.updateView();
   };
 
   isFormDirty() {
@@ -118,7 +120,7 @@ export default class SamlSettingsForm {
   }
 
   testButtonTooltip() {
-    if (!this.samlProviderEnabled) {
+    if (!this.getValueWithDeps('group-saml')) {
       return __('Group SAML must be enabled to test');
     }
 
@@ -129,6 +131,16 @@ export default class SamlSettingsForm {
     return __('Redirect to SAML provider to test configuration');
   }
 
+  disableTestButton() {
+    toggleElementVisibility(this.testButton, false);
+    toggleElementVisibility(this.testButtonDisabled, true);
+  }
+
+  enableTestButton() {
+    toggleElementVisibility(this.testButton, true);
+    toggleElementVisibility(this.testButtonDisabled, false);
+  }
+
   updateCheckboxes() {
     this.settings
       .filter((setting) => setting.dependsOn)
@@ -154,9 +166,9 @@ export default class SamlSettingsForm {
 
   updateView() {
     if (this.getValueWithDeps('group-saml') && !this.isFormDirty()) {
-      this.testButton.removeAttribute('disabled');
+      this.enableTestButton();
     } else {
-      this.testButton.setAttribute('disabled', true);
+      this.disableTestButton();
     }
 
     this.updateCheckboxes();

ee/app/views/groups/saml_providers/_test_button.html.haml

-= saml_link_for_provider _('Verify SAML Configuration'), saml_provider, redirect: ::OmniAuth::Strategies::GroupSaml::VERIFY_SAML_RESPONSE, html_class: "gl-button btn btn-default qa-saml-settings-test-button #{ 'd-none' unless saml_provider.persisted? }"
+= return unless saml_provider.persisted?
+
+= saml_link_for_provider _('Verify SAML Configuration'), saml_provider, redirect: ::OmniAuth::Strategies::GroupSaml::VERIFY_SAML_RESPONSE, html_class: "gl-button btn btn-default qa-saml-settings-test-button"
+%button.gl-button.btn.btn-default.qa-saml-settings-test-button.gl-display-none{ disabled: true }
+  = _('Verify SAML Configuration')

ee/spec/frontend/saml_providers/saml_settings_form_spec.js

@@ -11,93 +11,91 @@ describe('SamlSettingsForm', () => {
     samlSettingsForm.init();
   });
 
-  const findEnforcedGroupManagedAccountSetting = () =>
-    samlSettingsForm.settings.find((s) => s.name === 'enforced-group-managed-accounts');
+  const findGroupSamlSetting = () => samlSettingsForm.settings.find((s) => s.name === 'group-saml');
   const findEnforcedSsoSetting = () =>
     samlSettingsForm.settings.find((s) => s.name === 'enforced-sso');
-  const findProhibitForksSetting = () =>
-    samlSettingsForm.settings.find((s) => s.name === 'prohibited-outer-forks');
+  const findEnforcedGitActivity = () =>
+    samlSettingsForm.settings.find((s) => s.name === 'enforced-git-activity-check');
+
+  const fireChangeEvent = (setting) => {
+    setting.el.dispatchEvent(
+      new Event('change', {
+        bubbles: true,
+      }),
+    );
+  };
+
+  const checkSetting = (setting) => {
+    // eslint-disable-next-line no-param-reassign
+    setting.el.checked = true;
+
+    fireChangeEvent(setting);
+  };
+
+  const uncheckSetting = (setting) => {
+    // eslint-disable-next-line no-param-reassign
+    setting.el.checked = false;
+
+    fireChangeEvent(setting);
+  };
+
+  const expectTestButtonDisabled = () => {
+    expect(samlSettingsForm.testButtonDisabled.classList.contains('gl-display-none')).toBe(false);
+    expect(samlSettingsForm.testButton.classList.contains('gl-display-none')).toBe(true);
+  };
+  const expectTestButtonEnabled = () => {
+    expect(samlSettingsForm.testButtonDisabled.classList.contains('gl-display-none')).toBe(true);
+    expect(samlSettingsForm.testButton.classList.contains('gl-display-none')).toBe(false);
+  };
 
   describe('updateView', () => {
-    it('disables Test button when form has changes', () => {
-      samlSettingsForm.dirtyFormChecker.dirtyInputs = [findEnforcedGroupManagedAccountSetting().el];
+    it('disables Test button when form has changes and re-enables when returned to starting state', () => {
+      expectTestButtonEnabled();
 
-      expect(samlSettingsForm.testButton.hasAttribute('disabled')).toBe(false);
+      uncheckSetting(findEnforcedSsoSetting());
 
-      samlSettingsForm.updateView();
+      expectTestButtonDisabled();
 
-      expect(samlSettingsForm.testButton.hasAttribute('disabled')).toBe(true);
-    });
-
-    it('re-enables Test button when form is returned to starting state', () => {
-      samlSettingsForm.testButton.setAttribute('disabled', true);
+      checkSetting(findEnforcedSsoSetting());
 
-      samlSettingsForm.updateView();
-
-      expect(samlSettingsForm.testButton.hasAttribute('disabled')).toBe(false);
+      expectTestButtonEnabled();
     });
 
     it('keeps Test button disabled when SAML disabled for the group', () => {
-      samlSettingsForm.settings.find((s) => s.name === 'group-saml').value = false;
-      samlSettingsForm.testButton.setAttribute('disabled', true);
+      expectTestButtonEnabled();
 
-      samlSettingsForm.updateView();
+      uncheckSetting(findGroupSamlSetting());
 
-      expect(samlSettingsForm.testButton.hasAttribute('disabled')).toBe(true);
+      expectTestButtonDisabled();
     });
   });
 
   it('correctly disables dependent toggle and shows helper text', () => {
-    samlSettingsForm.settings.forEach((s) => {
-      const { el } = s;
-      el.checked = true;
-    });
+    expect(findEnforcedSsoSetting().el.hasAttribute('disabled')).toBe(false);
+    expect(findEnforcedSsoSetting().helperText.classList.contains('gl-display-none')).toBe(true);
 
-    samlSettingsForm.updateSAMLSettings();
-    samlSettingsForm.updateView();
-    expect(findProhibitForksSetting().el.hasAttribute('disabled')).toBe(false);
-    expect(findProhibitForksSetting().helperText.classList.contains('gl-display-none')).toBe(true);
+    uncheckSetting(findGroupSamlSetting());
 
-    findEnforcedGroupManagedAccountSetting().el.checked = false;
-    samlSettingsForm.updateSAMLSettings();
-    samlSettingsForm.updateView();
-
-    expect(findProhibitForksSetting().el.hasAttribute('disabled')).toBe(true);
-    expect(findProhibitForksSetting().helperText.classList.contains('gl-display-none')).toBe(false);
-    expect(findProhibitForksSetting().value).toBe(true);
+    expect(findEnforcedSsoSetting().el.hasAttribute('disabled')).toBe(true);
+    expect(findEnforcedSsoSetting().helperText.classList.contains('gl-display-none')).toBe(false);
+    expect(findEnforcedSsoSetting().value).toBe(true);
   });
 
   it('correctly shows warning text when checkbox is unchecked', () => {
     expect(findEnforcedSsoSetting().warning.classList.contains('gl-display-none')).toBe(true);
 
-    findEnforcedSsoSetting().el.checked = false;
-    samlSettingsForm.updateSAMLSettings();
-    samlSettingsForm.updateView();
+    uncheckSetting(findEnforcedSsoSetting());
 
     expect(findEnforcedSsoSetting().warning.classList.contains('gl-display-none')).toBe(false);
   });
 
   it('correctly disables multiple dependent toggles', () => {
-    samlSettingsForm.settings.forEach((s) => {
-      const { el } = s;
-      el.checked = true;
-    });
-
-    let groupSamlSetting;
-    let otherSettings;
-
-    samlSettingsForm.updateSAMLSettings();
-    samlSettingsForm.updateView();
-    [groupSamlSetting, ...otherSettings] = samlSettingsForm.settings;
-    expect(samlSettingsForm.settings.every((s) => s.value)).toBe(true);
-    expect(samlSettingsForm.settings.some((s) => s.el.hasAttribute('disabled'))).toBe(false);
+    expect(findEnforcedSsoSetting().el.hasAttribute('disabled')).toBe(false);
+    expect(findEnforcedGitActivity().el.hasAttribute('disabled')).toBe(false);
 
-    groupSamlSetting.el.checked = false;
-    samlSettingsForm.updateSAMLSettings();
-    samlSettingsForm.updateView();
+    uncheckSetting(findGroupSamlSetting());
 
-    [groupSamlSetting, ...otherSettings] = samlSettingsForm.settings;
-    expect(otherSettings.every((s) => s.value)).toBe(true);
-    expect(otherSettings.every((s) => s.el.hasAttribute('disabled'))).toBe(true);
+    expect(findEnforcedSsoSetting().el.hasAttribute('disabled')).toBe(true);
+    expect(findEnforcedGitActivity().el.hasAttribute('disabled')).toBe(true);
   });
 });

spec/frontend/dirty_submit/dirty_submit_form_spec.js

@@ -93,5 +93,38 @@ describe('DirtySubmitForm', () => {
 
       expect(updateDirtyInputSpy).toHaveBeenCalledTimes(range.length);
     });
+
+    describe('when inputs listener is added', () => {
+      it('calls listener when changes are made to an input', () => {
+        const { form, input } = createForm();
+        const inputsListener = jest.fn();
+
+        const dirtySubmitForm = new DirtySubmitForm(form);
+        dirtySubmitForm.addInputsListener(inputsListener);
+
+        setInputValue(input, 'new value');
+
+        jest.runOnlyPendingTimers();
+
+        expect(inputsListener).toHaveBeenCalledTimes(1);
+      });
+
+      describe('when inputs listener is removed', () => {
+        it('does not call listener when changes are made to an input', () => {
+          const { form, input } = createForm();
+          const inputsListener = jest.fn();
+
+          const dirtySubmitForm = new DirtySubmitForm(form);
+          dirtySubmitForm.addInputsListener(inputsListener);
+          dirtySubmitForm.removeInputsListener(inputsListener);
+
+          setInputValue(input, 'new value');
+
+          jest.runOnlyPendingTimers();
+
+          expect(inputsListener).not.toHaveBeenCalled();
+        });
+      });
+    });
   });
 });