SONAR-17295 Improving user experience for SAML authentication page

diff --git a/server/sonar-web/src/main/js/api/mocks/AuthenticationServiceMock.ts b/server/sonar-web/src/main/js/api/mocks/AuthenticationServiceMock.ts
index c0b5b8a8bccc0578cffe83a1f10e5f6f884a234d..17dda673074c7975ca189cb266dba1e7c70f5e4e 100644 (file)
--- a/server/sonar-web/src/main/js/api/mocks/AuthenticationServiceMock.ts
+++ b/server/sonar-web/src/main/js/api/mocks/AuthenticationServiceMock.ts
@@ -49,6 +49,14 @@ export default class AuthenticationServiceMock {
   };
 
   setValueHandler = (definition: SettingDefinition, value: string) => {
+    if (value === 'error') {
+      const res = new Response('', {
+        status: 400,
+        statusText: 'fail'
+      });
+
+      return Promise.reject(res);
+    }
     const updatedSettingValue = this.settingValues.find(set => set.key === definition.key);
     if (updatedSettingValue) {
       updatedSettingValue.value = value;
@@ -58,16 +66,14 @@ export default class AuthenticationServiceMock {
 
   resetValueHandler = (data: { keys: string; component?: string } & BranchParameters) => {
     if (data.keys) {
-      return Promise.resolve(
-        this.settingValues.map(set => {
-          if (data.keys.includes(set.key)) {
-            set.value = '';
-          }
-          return set;
-        })
-      );
+      this.settingValues.forEach(set => {
+        if (data.keys.includes(set.key)) {
+          set.value = '';
+        }
+        return set;
+      });
     }
-    return Promise.resolve(this.settingValues);
+    return Promise.resolve();
   };
 
   resetValues = () => {

diff --git a/server/sonar-web/src/main/js/apps/settings/components/authentication/SamlAuthentication.tsx b/server/sonar-web/src/main/js/apps/settings/components/authentication/SamlAuthentication.tsx
index e92b8333c061e1965f4377d32bd8d4ce1fb9f0d1..05a8a9b44bfffc7180c2addad7f5e5861d754ff7 100644 (file)
--- a/server/sonar-web/src/main/js/apps/settings/components/authentication/SamlAuthentication.tsx
+++ b/server/sonar-web/src/main/js/apps/settings/components/authentication/SamlAuthentication.tsx
@@ -23,10 +23,13 @@ import React from 'react';
 import { getValues, resetSettingValue, setSettingValue } from '../../../../api/settings';
 import { SubmitButton } from '../../../../components/controls/buttons';
 import Tooltip from '../../../../components/controls/Tooltip';
+import { Location, withRouter } from '../../../../components/hoc/withRouter';
+import AlertSuccessIcon from '../../../../components/icons/AlertSuccessIcon';
+import AlertWarnIcon from '../../../../components/icons/AlertWarnIcon';
 import DetachIcon from '../../../../components/icons/DetachIcon';
 import DeferredSpinner from '../../../../components/ui/DeferredSpinner';
-import { translate } from '../../../../helpers/l10n';
-import { parseError } from '../../../../helpers/request';
+import { translate, translateWithParameters } from '../../../../helpers/l10n';
+import { isSuccessStatus, parseError } from '../../../../helpers/request';
 import { getBaseUrl } from '../../../../helpers/system';
 import { ExtendedSettingDefinition, SettingType, SettingValue } from '../../../../types/settings';
 import SamlFormField from './SamlFormField';
@@ -34,6 +37,7 @@ import SamlToggleField from './SamlToggleField';
 
 interface SamlAuthenticationProps {
   definitions: ExtendedSettingDefinition[];
+  location: Location;
 }
 
 interface SamlAuthenticationState {
@@ -42,6 +46,7 @@ interface SamlAuthenticationState {
   dirtyFields: string[];
   securedFieldsSubmitted: string[];
   error: { [key: string]: string };
+  success?: boolean;
 }
 
 const CONFIG_TEST_PATH = '/api/saml/validation_init';
@@ -60,6 +65,8 @@ class SamlAuthentication extends React.PureComponent<
   SamlAuthenticationProps,
   SamlAuthenticationState
 > {
+  formFieldRef: React.RefObject<HTMLDivElement> = React.createRef();
+
   constructor(props: SamlAuthenticationProps) {
     super(props);
     const settingValue = props.definitions.map(def => {
@@ -83,6 +90,17 @@ class SamlAuthentication extends React.PureComponent<
     this.loadSettingValues(keys);
   }
 
+  componentDidUpdate(prevProps: SamlAuthenticationProps) {
+    const { location } = this.props;
+    if (this.formFieldRef.current && prevProps.location.hash !== location.hash) {
+      this.formFieldRef.current.scrollIntoView({
+        behavior: 'smooth',
+        block: 'center',
+        inline: 'nearest'
+      });
+    }
+  }
+
   onFieldChange = (id: string, value: string | boolean) => {
     const { settingValue, dirtyFields } = this.state;
     const updatedSettingValue = settingValue?.map(set => {
@@ -151,52 +169,50 @@ class SamlAuthentication extends React.PureComponent<
       return;
     }
 
-    this.setState({ submitting: true, error: {} });
+    this.setState({ submitting: true, error: {}, success: false });
     const promises: Promise<void>[] = [];
 
-    settingValue?.forEach(set => {
-      const definition = definitions.find(def => def.key === set.key);
-      if (definition && set.value !== undefined && dirtyFields.includes(set.key)) {
+    dirtyFields.forEach(field => {
+      const definition = definitions.find(def => def.key === field);
+      const value = settingValue.find(def => def.key === field)?.value;
+      if (definition && value !== undefined) {
         const apiCall =
-          set.value.length > 0
-            ? setSettingValue(definition, set.value)
+          value.length > 0
+            ? setSettingValue(definition, value)
             : resetSettingValue({ keys: definition.key });
-        const promise = apiCall.catch(async e => {
+
+        promises.push(apiCall);
+      }
+    });
+
+    await Promise.all(promises.map(p => p.catch(e => e))).then(data => {
+      const dataWithError = data
+        .map((data, index) => ({ data, index }))
+        .filter(d => d.data !== undefined && !isSuccessStatus(d.data.status));
+      if (dataWithError.length > 0) {
+        dataWithError.forEach(async d => {
+          const validationMessage = await parseError(d.data as Response);
           const { error } = this.state;
-          const validationMessage = await parseError(e as Response);
           this.setState({
-            submitting: false,
-            dirtyFields: [],
-            error: { ...error, ...{ [set.key]: validationMessage } }
+            error: { ...error, ...{ [dirtyFields[d.index]]: validationMessage } }
           });
         });
-        promises.push(promise);
       }
+      this.setState({ success: dirtyFields.length !== dataWithError.length });
     });
-    await Promise.all(promises);
     await this.loadSettingValues(dirtyFields.join(','));
-
     this.setState({ submitting: false, dirtyFields: [] });
   };
 
   allowEnabling = () => {
-    const { settingValue, securedFieldsSubmitted } = this.state;
+    const { settingValue } = this.state;
     const enabledFlagSettingValue = settingValue.find(set => set.key === SAML_ENABLED_FIELD);
+
     if (enabledFlagSettingValue && enabledFlagSettingValue.value === 'true') {
       return true;
     }
 
-    for (const setting of settingValue) {
-      const isMandatory = !OPTIONAL_FIELDS.includes(setting.key);
-      const isSecured = this.isSecuredField(setting.key);
-      const isSecuredAndNotSubmitted = isSecured && !securedFieldsSubmitted.includes(setting.key);
-      const isNotSecuredAndNotSubmitted =
-        !isSecured && (setting.value === '' || setting.value === undefined);
-      if (isMandatory && (isSecuredAndNotSubmitted || isNotSecuredAndNotSubmitted)) {
-        return false;
-      }
-    }
-    return true;
+    return this.getEmptyRequiredFields().length === 0;
   };
 
   onEnableFlagChange = (value: boolean) => {
@@ -232,13 +248,43 @@ class SamlAuthentication extends React.PureComponent<
     return null;
   };
 
+  getEmptyRequiredFields = () => {
+    const { settingValue, securedFieldsSubmitted } = this.state;
+    const { definitions } = this.props;
+
+    const updatedRequiredFields: string[] = [];
+
+    for (const setting of settingValue) {
+      const isMandatory = !OPTIONAL_FIELDS.includes(setting.key);
+      const isSecured = this.isSecuredField(setting.key);
+      const isSecuredAndNotSubmitted = isSecured && !securedFieldsSubmitted.includes(setting.key);
+      const isNotSecuredAndNotSubmitted =
+        !isSecured && (setting.value === '' || setting.value === undefined);
+      if (isMandatory && (isSecuredAndNotSubmitted || isNotSecuredAndNotSubmitted)) {
+        const settingDef = definitions.find(def => def.key === setting.key);
+
+        if (settingDef && settingDef.name) {
+          updatedRequiredFields.push(settingDef.name);
+        }
+      }
+    }
+    return updatedRequiredFields;
+  };
+
   render() {
     const { definitions } = this.props;
-    const { submitting, settingValue, securedFieldsSubmitted, error, dirtyFields } = this.state;
+    const {
+      submitting,
+      settingValue,
+      securedFieldsSubmitted,
+      error,
+      dirtyFields,
+      success
+    } = this.state;
     const enabledFlagDefinition = definitions.find(def => def.key === SAML_ENABLED_FIELD);
 
     const formIsIncomplete = !this.allowEnabling();
-    const preventTestingConfig = formIsIncomplete || dirtyFields.length > 0;
+    const preventTestingConfig = this.getEmptyRequiredFields().length > 0 || dirtyFields.length > 0;
 
     return (
       <div>
@@ -247,32 +293,71 @@ class SamlAuthentication extends React.PureComponent<
             return null;
           }
           return (
-            <SamlFormField
-              settingValue={settingValue?.find(set => set.key === def.key)}
-              definition={def}
-              mandatory={!OPTIONAL_FIELDS.includes(def.key)}
-              onFieldChange={this.onFieldChange}
-              showSecuredTextArea={
-                !securedFieldsSubmitted.includes(def.key) || dirtyFields.includes(def.key)
-              }
-              error={error}
+            <div
               key={def.key}
-            />
+              ref={this.props.location.hash.substring(1) === def.key ? this.formFieldRef : null}>
+              <SamlFormField
+                settingValue={settingValue?.find(set => set.key === def.key)}
+                definition={def}
+                mandatory={!OPTIONAL_FIELDS.includes(def.key)}
+                onFieldChange={this.onFieldChange}
+                showSecuredTextArea={
+                  !securedFieldsSubmitted.includes(def.key) || dirtyFields.includes(def.key)
+                }
+                error={error}
+              />
+            </div>
           );
         })}
-        <div className="fixed-footer padded-left padded-right">
+        <div className="fixed-footer padded">
           {enabledFlagDefinition && (
-            <div>
-              <label className="h3 spacer-right">{enabledFlagDefinition.name}</label>
-              <SamlToggleField
-                definition={enabledFlagDefinition}
-                settingValue={settingValue?.find(set => set.key === enabledFlagDefinition.key)}
-                toggleDisabled={formIsIncomplete}
-                onChange={this.onEnableFlagChange}
-              />
-            </div>
+            <Tooltip
+              overlay={
+                this.allowEnabling()
+                  ? null
+                  : translateWithParameters(
+                      'settings.authentication.saml.tooltip.required_fields',
+                      this.getEmptyRequiredFields().join(', ')
+                    )
+              }>
+              <div className="display-inline-flex-center">
+                <label className="h3 spacer-right">{enabledFlagDefinition.name}</label>
+                <SamlToggleField
+                  definition={enabledFlagDefinition}
+                  settingValue={settingValue?.find(set => set.key === enabledFlagDefinition.key)}
+                  toggleDisabled={formIsIncomplete}
+                  onChange={this.onEnableFlagChange}
+                />
+              </div>
+            </Tooltip>
           )}
-          <div>
+          <div className="display-inline-flex-center">
+            {success && (
+              <div className="spacer-right">
+                <Tooltip
+                  overlay={
+                    Object.keys(error).length > 0
+                      ? translateWithParameters(
+                          'settings.authentication.saml.form.save_warn',
+                          Object.keys(error).length
+                        )
+                      : null
+                  }>
+                  {Object.keys(error).length > 0 ? (
+                    <span>
+                      <AlertWarnIcon className="spacer-right" />
+                      {translate('settings.authentication.saml.form.save_partial')}
+                    </span>
+                  ) : (
+                    <span>
+                      <AlertSuccessIcon className="spacer-right" />
+                      {translate('settings.authentication.saml.form.save_success')}
+                    </span>
+                  )}
+                  {}
+                </Tooltip>
+              </div>
+            )}
             <SubmitButton className="button-primary spacer-right" onClick={this.onSaveConfig}>
               {translate('settings.authentication.saml.form.save')}
               <DeferredSpinner className="spacer-left" loading={submitting} />
@@ -298,4 +383,4 @@ class SamlAuthentication extends React.PureComponent<
   }
 }
 
-export default SamlAuthentication;
+export default withRouter(SamlAuthentication);

diff --git a/server/sonar-web/src/main/js/apps/settings/components/authentication/SamlSecuredField.tsx b/server/sonar-web/src/main/js/apps/settings/components/authentication/SamlSecuredField.tsx
index 4cc6353b001ed42540f923aa19bc5630fc12e18c..b78ca353f7431e89d03ae7f4ca3aa46c05fcfd89 100644 (file)
--- a/server/sonar-web/src/main/js/apps/settings/components/authentication/SamlSecuredField.tsx
+++ b/server/sonar-web/src/main/js/apps/settings/components/authentication/SamlSecuredField.tsx
@@ -44,7 +44,7 @@ export default function SamlSecuredField(props: SamlToggleFieldProps) {
         <textarea
           className="width-100"
           id={definition.key}
-          maxLength={2000}
+          maxLength={4000}
           onChange={e => props.onFieldChange(definition.key, e.currentTarget.value)}
           required={!optional}
           rows={5}

diff --git a/server/sonar-web/src/main/js/apps/settings/components/authentication/__tests__/Authentication-test.tsx b/server/sonar-web/src/main/js/apps/settings/components/authentication/__tests__/Authentication-test.tsx
index dded249d99f99530c6ae209ba9055a1217d7651d..7027cbef4e40b4f3aaaaef10c6e820c450ecaab2 100644 (file)
--- a/server/sonar-web/src/main/js/apps/settings/components/authentication/__tests__/Authentication-test.tsx
+++ b/server/sonar-web/src/main/js/apps/settings/components/authentication/__tests__/Authentication-test.tsx
@@ -28,6 +28,39 @@ import Authentication from '../Authentication';
 
 jest.mock('../../../../../api/settings');
 
+const mockDefinitionFields = [
+  mockDefinition({
+    key: 'test1',
+    category: 'authentication',
+    subCategory: 'saml',
+    name: 'test1',
+    description: 'desc1'
+  }),
+  mockDefinition({
+    key: 'test2',
+    category: 'authentication',
+    subCategory: 'saml',
+    name: 'test2',
+    description: 'desc2'
+  }),
+  mockDefinition({
+    key: 'sonar.auth.saml.certificate.secured',
+    category: 'authentication',
+    subCategory: 'saml',
+    name: 'Certificate',
+    description: 'Secured certificate',
+    type: SettingType.PASSWORD
+  }),
+  mockDefinition({
+    key: 'sonar.auth.saml.enabled',
+    category: 'authentication',
+    subCategory: 'saml',
+    name: 'Enabled',
+    description: 'To enable the flag',
+    type: SettingType.BOOLEAN
+  })
+];
+
 let handler: AuthenticationServiceMock;
 
 beforeEach(() => {
@@ -91,38 +124,7 @@ it('should allow user to test the configuration', async () => {
 
 it('should allow user to edit fields and save configuration', async () => {
   const user = userEvent.setup();
-  const definitions = [
-    mockDefinition({
-      key: 'test1',
-      category: 'authentication',
-      subCategory: 'saml',
-      name: 'test1',
-      description: 'desc1'
-    }),
-    mockDefinition({
-      key: 'test2',
-      category: 'authentication',
-      subCategory: 'saml',
-      name: 'test2',
-      description: 'desc2'
-    }),
-    mockDefinition({
-      key: 'sonar.auth.saml.certificate.secured',
-      category: 'authentication',
-      subCategory: 'saml',
-      name: 'Certificate',
-      description: 'Secured certificate',
-      type: SettingType.PASSWORD
-    }),
-    mockDefinition({
-      key: 'sonar.auth.saml.enabled',
-      category: 'authentication',
-      subCategory: 'saml',
-      name: 'Enabled',
-      description: 'To enable the flag',
-      type: SettingType.BOOLEAN
-    })
-  ];
+  const definitions = mockDefinitionFields;
   renderAuthentication(definitions);
 
   expect(screen.getByRole('button', { name: 'off' })).toHaveAttribute('aria-disabled', 'true');
@@ -160,6 +162,7 @@ it('should allow user to edit fields and save configuration', async () => {
   expect(screen.getByRole('button', { name: 'on' })).toBeInTheDocument();
 
   await user.click(screen.getByRole('button', { name: 'settings.authentication.saml.form.save' }));
+  expect(screen.getByText('settings.authentication.saml.form.save_success')).toBeInTheDocument();
   // check after switching tab that the flag is still enabled
   await user.click(screen.getByRole('tab', { name: 'github GitHub' }));
   await user.click(screen.getByRole('tab', { name: 'SAML' }));
@@ -167,6 +170,19 @@ it('should allow user to edit fields and save configuration', async () => {
   expect(screen.getByRole('button', { name: 'on' })).toBeInTheDocument();
 });
 
+it('should handle and show error to the user', async () => {
+  const user = userEvent.setup();
+  const definitions = mockDefinitionFields;
+  renderAuthentication(definitions);
+
+  await user.click(screen.getByRole('textbox', { name: 'test1' }));
+  await user.keyboard('value');
+  await user.click(screen.getByRole('textbox', { name: 'test2' }));
+  await user.keyboard('{Control>}a{/Control}error');
+  await user.click(screen.getByRole('button', { name: 'settings.authentication.saml.form.save' }));
+  expect(screen.getByText('settings.authentication.saml.form.save_partial')).toBeInTheDocument();
+});
+
 function renderAuthentication(definitions: ExtendedSettingDefinition[]) {
   renderComponent(<Authentication definitions={definitions} />);
 }

diff --git a/server/sonar-web/src/main/js/apps/settings/styles.css b/server/sonar-web/src/main/js/apps/settings/styles.css
index d497771de73b0bf990591f0f291d3cf10afa4e2f..e4caa3c42d8beba26ff863bcece7c13e4bd0bcc1 100644 (file)
--- a/server/sonar-web/src/main/js/apps/settings/styles.css
+++ b/server/sonar-web/src/main/js/apps/settings/styles.css
@@ -203,7 +203,6 @@
 .fixed-footer {
   position: sticky;
   bottom: 0px;
-  height: 50px;
   align-items: center;
   display: flex;
   border: 1px solid var(--gray80);

diff --git a/sonar-core/src/main/resources/org/sonar/l10n/core.properties b/sonar-core/src/main/resources/org/sonar/l10n/core.properties
index fdb9dbf0cc24498ed5b1ac056739618543bc4f13..75584c852e3403c0832ed7a44db8d2b7a700f88c 100644 (file)
--- a/sonar-core/src/main/resources/org/sonar/l10n/core.properties
+++ b/sonar-core/src/main/resources/org/sonar/l10n/core.properties
@@ -1269,6 +1269,10 @@ settings.authentication.saml.form.save=Save configuration
 settings.authentication.saml.form.test=Test configuration
 settings.authentication.saml.form.test.help.dirty=You must save your changes
 settings.authentication.saml.form.test.help.incomplete=Some mandatory fields are empty
+settings.authentication.saml.form.save_success=Saved successfully
+settings.authentication.saml.form.save_partial=Saved partially
+settings.authentication.saml.form.save_warn=Please check the error messages in the form above, saving failed for {0} field(s).
+settings.authentication.saml.tooltip.required_fields=Please provide a value for the following required field(s): {0}
 
 settings.pr_decoration.binding.category=DevOps Platform Integration
 settings.pr_decoration.binding.no_bindings=A system administrator needs to enable this feature in the global settings.