Improve UX of SAML Settings

Do not reset toggles to "false" when disabling

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

@@ -58,8 +58,23 @@ export default class SamlSettingsForm {
     this.dirtyFormChecker = new DirtyFormChecker(formSelector, () => this.updateView());
   }
 
-  getSettingValue(name) {
-    return this.settings.find(s => s.name === name).value;
+  findSetting(name) {
+    return this.settings.find(s => s.name === name);
+  }
+
+  getValueWithDeps(name) {
+    const setting = this.findSetting(name);
+    let currentDependsOn = setting.dependsOn;
+
+    while (currentDependsOn) {
+      const { value, dependsOn } = this.findSetting(currentDependsOn);
+      if (!value) {
+        return false;
+      }
+      currentDependsOn = dependsOn;
+    }
+
+    return setting.value;
   }
 
   init() {
@@ -95,28 +110,26 @@ export default class SamlSettingsForm {
   }
 
   updateToggles() {
-    this.settings.forEach(setting => {
-      const { helperText, callout, toggle } = setting;
-      if (setting.dependsOn) {
-        const dependencyToggleValue = this.getSettingValue(setting.dependsOn);
+    this.settings
+      .filter(setting => setting.dependsOn)
+      .forEach(setting => {
+        const { helperText, callout, toggle } = setting;
+        const dependentToggleValue = this.getValueWithDeps(setting.dependsOn);
         if (helperText) {
-          helperText.style.display = dependencyToggleValue ? 'none' : 'block';
+          helperText.style.display = dependentToggleValue ? 'none' : 'block';
         }
 
-        if (!dependencyToggleValue && setting.value) {
-          setting.toggle.click();
-        }
-        toggle.disabled = !dependencyToggleValue;
-      }
+        toggle.classList.toggle('is-disabled', dependentToggleValue);
+        toggle.disabled = !dependentToggleValue;
 
-      if (callout) {
-        callout.style.display = setting.value ? 'block' : 'none';
-      }
-    });
+        if (callout) {
+          callout.style.display = setting.value && dependentToggleValue ? 'block' : 'none';
+        }
+      });
   }
 
   updateView() {
-    if (this.getSettingValue('group-saml') && !this.dirtyFormChecker.isDirty) {
+    if (this.getValueWithDeps('group-saml') && !this.dirtyFormChecker.isDirty) {
       this.testButton.removeAttribute('disabled');
     } else {
       this.testButton.setAttribute('disabled', true);

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

@@ -9,7 +9,7 @@
     - if Feature.enabled?(:enforced_sso, group)
       .form-group
         %label.toggle-wrapper.mb-0.js-group-saml-enforced-sso-toggle-area
-          = render "shared/buttons/project_feature_toggle", is_checked: saml_provider.enforced_sso?, label: s_("GroupSAML|Enforced SSO"), class_list: "js-project-feature-toggle js-group-saml-enforced-sso-toggle project-feature-toggle d-inline", data: { qa_selector: 'enforced_sso_toggle_button' } do
+          = render "shared/buttons/project_feature_toggle", is_checked: saml_provider.enforced_sso, disabled: !saml_provider.enabled?, label: s_("GroupSAML|Enforced SSO"), class_list: "js-project-feature-toggle js-group-saml-enforced-sso-toggle project-feature-toggle d-inline", data: { qa_selector: 'enforced_sso_toggle_button' } do
             = f.hidden_field :enforced_sso, { class: 'js-group-saml-enforced-sso-input js-project-feature-toggle-input'}
           %span.form-text.d-inline.font-weight-normal.align-text-bottom.ml-3= Feature.enabled?(:enforced_sso_requires_session, group) ? s_('GroupSAML|Enforce SSO-only authentication for this group.') : s_('GroupSAML|Enforce SSO-only membership for this group.')
         .form-text.text-muted.js-helper-text{ style: "display: #{'none' if saml_provider.enabled?} #{'block' unless saml_provider.enabled?}" }
@@ -18,7 +18,7 @@
     - if Feature.enabled?(:group_managed_accounts, group)
       .form-group
         %label.toggle-wrapper.mb-0.js-group-saml-enforced-group-managed-accounts-toggle-area
-          = render "shared/buttons/project_feature_toggle", is_checked: saml_provider.enforced_group_managed_accounts?, label: s_("GroupSAML|Enforced SSO"), class_list: "js-project-feature-toggle js-group-saml-enforced-group-managed-accounts-toggle project-feature-toggle d-inline", data: { qa_selector: 'group_managed_accounts_toggle_button' } do
+          = render "shared/buttons/project_feature_toggle", is_checked: saml_provider.enforced_group_managed_accounts, disabled: !saml_provider.enforced_sso?, label: s_("GroupSAML|Enforced SSO"), class_list: "js-project-feature-toggle js-group-saml-enforced-group-managed-accounts-toggle project-feature-toggle d-inline", data: { qa_selector: 'group_managed_accounts_toggle_button' } do
             = f.hidden_field :enforced_group_managed_accounts, { class: 'js-group-saml-enforced-group-managed-accounts-input js-project-feature-toggle-input'}
           %span.form-text.d-inline.font-weight-normal.align-text-bottom.ml-3= s_('GroupSAML|Enforce users to have dedicated group managed accounts for this group.')
         .form-text.text-muted.js-helper-text{ style: "display: #{'none' if saml_provider.enforced_sso?} #{'block' unless saml_provider.enforced_sso?}" }
@@ -28,7 +28,7 @@
           = s_('GroupSAML|With group managed accounts enabled, all the users without a group managed account will be excluded from the group.')
       .form-group
         %label.toggle-wrapper.mb-0.js-group-saml-prohibited-outer-forks-toggle-area
-          = render "shared/buttons/project_feature_toggle", is_checked: saml_provider.prohibited_outer_forks?, label: s_("GroupSAML|Prohibit outer forks"), class_list: "js-project-feature-toggle js-group-saml-prohibited-outer-forks-toggle project-feature-toggle d-inline", data: { qa_selector: 'prohibited_outer_forks_toggle_button' } do
+          = render "shared/buttons/project_feature_toggle", is_checked: saml_provider.prohibited_outer_forks, disabled: !saml_provider.enforced_group_managed_accounts?, label: s_("GroupSAML|Prohibit outer forks"), class_list: "js-project-feature-toggle js-group-saml-prohibited-outer-forks-toggle project-feature-toggle d-inline", data: { qa_selector: 'prohibited_outer_forks_toggle_button' } do
             = f.hidden_field :prohibited_outer_forks, { class: 'js-group-saml-prohibited-outer-forks-input js-project-feature-toggle-input'}
           %span.form-text.d-inline.font-weight-normal.align-text-bottom.ml-3= s_('GroupSAML|Prohibit outer forks for this group.')
         .form-text.text-muted.js-helper-text{ style: "display: #{'none' if saml_provider.enforced_group_managed_accounts?} #{'block' unless saml_provider.enforced_group_managed_accounts?}" }

ee/spec/frontend/saml_providers/saml_settings_form_spec.js

@@ -40,53 +40,52 @@ describe('SamlSettingsForm', () => {
     });
   });
 
-  it('correctly turns off dependent toggle', () => {
+  it('correctly disables dependent toggle', () => {
     samlSettingsForm.settings.forEach(s => {
       const { input } = s;
       input.value = true;
     });
 
-    const enforcedGroupManagedAccountSetting = samlSettingsForm.settings.find(
-      s => s.name === 'enforced-group-managed-accounts',
-    );
-    const prohibitForksSetting = samlSettingsForm.settings.find(
-      s => s.name === 'prohibited-outer-forks',
-    );
+    const findEnforcedGroupManagedAccountSetting = () =>
+      samlSettingsForm.settings.find(s => s.name === 'enforced-group-managed-accounts');
+    const findProhibitForksSetting = () =>
+      samlSettingsForm.settings.find(s => s.name === 'prohibited-outer-forks');
 
     samlSettingsForm.updateSAMLSettings();
     samlSettingsForm.updateView();
-    expect(prohibitForksSetting.toggle.hasAttribute('disabled')).toBe(false);
+    expect(findProhibitForksSetting().toggle.hasAttribute('disabled')).toBe(false);
 
-    enforcedGroupManagedAccountSetting.input.value = false;
+    findEnforcedGroupManagedAccountSetting().input.value = false;
     samlSettingsForm.updateSAMLSettings();
     samlSettingsForm.updateView();
 
-    expect(prohibitForksSetting.toggle.hasAttribute('disabled')).toBe(true);
-    expect(prohibitForksSetting.value).toBe(false);
+    expect(findProhibitForksSetting().toggle.hasAttribute('disabled')).toBe(true);
+    expect(findProhibitForksSetting().value).toBe(true);
   });
 
-  it('correctly turns off multiple dependent toggles', () => {
+  it('correctly disables multiple dependent toggles', () => {
     samlSettingsForm.settings.forEach(s => {
       const { input } = s;
       input.value = true;
     });
 
-    const [groupSamlSetting, ...otherSettings] = samlSettingsForm.settings;
+    let groupSamlSetting;
+    let otherSettings;
 
     samlSettingsForm.updateSAMLSettings();
     samlSettingsForm.updateView();
-    expect(samlSettingsForm.settings.map(s => s.value)).not.toContain(false);
-    expect(samlSettingsForm.settings.map(s => s.toggle.hasAttribute('disabled'))).not.toContain(
-      true,
-    );
+    [groupSamlSetting, ...otherSettings] = samlSettingsForm.settings;
+    expect(samlSettingsForm.settings.every(s => s.value)).toBe(true);
+    expect(samlSettingsForm.settings.some(s => s.toggle.hasAttribute('disabled'))).toBe(false);
 
     groupSamlSetting.input.value = false;
     samlSettingsForm.updateSAMLSettings();
     samlSettingsForm.updateView();
 
     return new Promise(window.requestAnimationFrame).then(() => {
-      expect(samlSettingsForm.settings.map(s => s.value)).not.toContain(true);
-      expect(otherSettings.map(s => s.toggle.hasAttribute('disabled'))).not.toContain(false);
+      [groupSamlSetting, ...otherSettings] = samlSettingsForm.settings;
+      expect(otherSettings.every(s => s.value)).toBe(true);
+      expect(otherSettings.every(s => s.toggle.hasAttribute('disabled'))).toBe(true);
     });
   });
 });