3.0 Group by selector on Configuration page always shows None after refresh (#410)

Signed-off-by: Kishore Kumaar Natarajan <kkumaarn@amazon.com>

Author: KishoreKicha14

common/constants.ts

@@ -18,10 +18,7 @@ export const GROUP_BY = 'Group by';
 export const AVERAGE_LATENCY = 'Average Latency';
 export const AVERAGE_CPU_TIME = 'Average CPU Time';
 export const AVERAGE_MEMORY_USAGE = 'Average Memory Usage';
-/*
- * Copyright OpenSearch Contributors
- * SPDX-License-Identifier: Apache-2.0
- */
+
 export const MetricType = {
   LATENCY: 'latency',
   CPU: 'cpu',
@@ -69,3 +66,18 @@ export const DEFAULT_TIME_UNIT = TIME_UNIT.MINUTES;
 export const DEFAULT_GROUP_BY = 'none';
 export const DEFAULT_EXPORTER_TYPE = EXPORTER_TYPE.localIndex;
 export const DEFAULT_DELETE_AFTER_DAYS = '7';
+// Validation constants
+export const VALIDATION_LIMITS = {
+  TOP_N_SIZE: {
+    MIN: 1,
+    MAX: 100,
+  },
+  WINDOW_SIZE_HOURS: {
+    MIN: 1,
+    MAX: 24,
+  },
+  DELETE_AFTER_DAYS: {
+    MIN: 1,
+    MAX: 180,
+  },
+};

public/pages/Configuration/Configuration.test.tsx

@@ -9,6 +9,13 @@ import '@testing-library/jest-dom/extend-expect';
 import { MemoryRouter } from 'react-router-dom';
 import Configuration from './Configuration';
 import { DataSourceContext } from '../TopNQueries/TopNQueries';
+import { TIME_UNITS_TEXT, EXPORTER_TYPE } from '../../../common/constants';
+import {
+  validateTopNSize,
+  validateWindowSize,
+  validateDeleteAfterDays,
+  validateConfiguration,
+} from './configurationValidation';
 
 const mockConfigInfo = jest.fn();
 const mockCoreStart = {
@@ -145,4 +152,183 @@ describe('Configuration Component', () => {
     fireEvent.click(screen.getByText('Cancel'));
     expect(getTopNSizeConfiguration()[0]).toHaveValue(5); // Resets to initial value
   });
+
+  describe('Validation Logic Tests', () => {
+    describe('TopN Size Validation', () => {
+      it('should hide Save button when topN size is less than 1', () => {
+        renderConfiguration();
+        fireEvent.change(getTopNSizeConfiguration()[0], { target: { value: '0' } });
+        expect(screen.queryByText('Save')).not.toBeInTheDocument();
+      });
+
+      it('should hide Save button when topN size is greater than 100', () => {
+        renderConfiguration();
+        fireEvent.change(getTopNSizeConfiguration()[0], { target: { value: '101' } });
+        expect(screen.queryByText('Save')).not.toBeInTheDocument();
+      });
+
+      it('should show Save button when topN size is within valid range (1-100)', () => {
+        renderConfiguration();
+        fireEvent.change(getTopNSizeConfiguration()[0], { target: { value: '50' } });
+        expect(screen.getByText('Save')).toBeInTheDocument();
+      });
+    });
+
+    describe('Window Size Validation - Minutes', () => {
+      it('should hide Save button when window size is empty for minutes', () => {
+        renderConfiguration();
+        fireEvent.change(getTopNSizeConfiguration()[0], { target: { value: '6' } });
+        fireEvent.change(getWindowSizeConfigurations()[2], { target: { value: 'MINUTES' } });
+        fireEvent.change(getWindowSizeConfigurations()[1], { target: { value: '' } });
+        expect(screen.queryByText('Save')).not.toBeInTheDocument();
+      });
+
+      it('should hide Save button when window size is NaN for minutes', () => {
+        renderConfiguration();
+        fireEvent.change(getTopNSizeConfiguration()[0], { target: { value: '6' } });
+        fireEvent.change(getWindowSizeConfigurations()[2], { target: { value: 'MINUTES' } });
+        fireEvent.change(getWindowSizeConfigurations()[1], { target: { value: 'invalid' } });
+        expect(screen.queryByText('Save')).not.toBeInTheDocument();
+      });
+
+      it('should show Save button when window size is valid for minutes', () => {
+        renderConfiguration();
+        fireEvent.change(getTopNSizeConfiguration()[0], { target: { value: '6' } });
+        fireEvent.change(getWindowSizeConfigurations()[2], { target: { value: 'MINUTES' } });
+        fireEvent.change(getWindowSizeConfigurations()[1], { target: { value: '5' } });
+        expect(screen.getByText('Save')).toBeInTheDocument();
+      });
+    });
+
+    describe('Window Size Validation - Hours', () => {
+      it('should show Save button when window size is within valid range (1-24) for hours', () => {
+        renderConfiguration();
+        fireEvent.change(getTopNSizeConfiguration()[0], { target: { value: '6' } });
+        fireEvent.change(getWindowSizeConfigurations()[2], { target: { value: 'HOURS' } });
+        fireEvent.change(getWindowSizeConfigurations()[1], { target: { value: '12' } });
+        expect(screen.getByText('Save')).toBeInTheDocument();
+      });
+    });
+
+    describe('Delete After Days Validation', () => {
+      it('should hide Save button when delete after days is less than 1 for local index', () => {
+        renderConfiguration();
+        fireEvent.change(getTopNSizeConfiguration()[0], { target: { value: '6' } });
+        const deleteAfterField = getTopNSizeConfiguration()[1];
+        fireEvent.change(deleteAfterField, { target: { value: '0' } });
+        expect(screen.queryByText('Save')).not.toBeInTheDocument();
+      });
+
+      it('should hide Save button when delete after days is greater than 180 for local index', () => {
+        renderConfiguration();
+        fireEvent.change(getTopNSizeConfiguration()[0], { target: { value: '6' } });
+        const deleteAfterField = getTopNSizeConfiguration()[1];
+        fireEvent.change(deleteAfterField, { target: { value: '181' } });
+        expect(screen.queryByText('Save')).not.toBeInTheDocument();
+      });
+
+      it('should show Save button when delete after days is within valid range (1-180) for local index', () => {
+        renderConfiguration();
+        fireEvent.change(getTopNSizeConfiguration()[0], { target: { value: '6' } });
+        const deleteAfterField = getTopNSizeConfiguration()[1];
+        fireEvent.change(deleteAfterField, { target: { value: '90' } });
+        expect(screen.getByText('Save')).toBeInTheDocument();
+      });
+
+      it('should show Save button when exporter is changed to none', () => {
+        renderConfiguration();
+        fireEvent.change(getTopNSizeConfiguration()[0], { target: { value: '6' } });
+        fireEvent.change(screen.getByDisplayValue('Local Index'), { target: { value: 'none' } });
+        expect(screen.getByText('Save')).toBeInTheDocument();
+      });
+    });
+
+    describe('Combined Validation Scenarios', () => {
+      it('should hide Save button when multiple validation rules fail', () => {
+        renderConfiguration();
+        fireEvent.change(getTopNSizeConfiguration()[0], { target: { value: '101' } });
+        expect(screen.queryByText('Save')).not.toBeInTheDocument();
+      });
+
+      it('should show Save button when all validation rules pass', () => {
+        renderConfiguration();
+        fireEvent.change(getTopNSizeConfiguration()[0], { target: { value: '25' } });
+        expect(screen.getByText('Save')).toBeInTheDocument();
+      });
+    });
+  });
+
+  describe('Validation Utility Functions', () => {
+    describe('validateTopNSize', () => {
+      it('should return false for values less than 1', () => {
+        expect(validateTopNSize('0')).toBe(false);
+        expect(validateTopNSize('-1')).toBe(false);
+      });
+
+      it('should return false for values greater than 100', () => {
+        expect(validateTopNSize('101')).toBe(false);
+        expect(validateTopNSize('200')).toBe(false);
+      });
+
+      it('should return true for valid values (1-100)', () => {
+        expect(validateTopNSize('1')).toBe(true);
+        expect(validateTopNSize('50')).toBe(true);
+        expect(validateTopNSize('100')).toBe(true);
+      });
+
+      it('should return false for non-numeric strings', () => {
+        expect(validateTopNSize('abc')).toBe(false);
+        expect(validateTopNSize('')).toBe(false);
+        expect(validateTopNSize('10.5')).toBe(false);
+      });
+    });
+
+    describe('validateWindowSize', () => {
+      it('should validate minutes correctly', () => {
+        const minutesUnit = TIME_UNITS_TEXT[0].value;
+        expect(validateWindowSize('', minutesUnit)).toBe(false);
+        expect(validateWindowSize('abc', minutesUnit)).toBe(false);
+        expect(validateWindowSize('1', minutesUnit)).toBe(true);
+        expect(validateWindowSize('30', minutesUnit)).toBe(true);
+      });
+
+      it('should validate hours correctly', () => {
+        const hoursUnit = TIME_UNITS_TEXT[1].value;
+        expect(validateWindowSize('0', hoursUnit)).toBe(false);
+        expect(validateWindowSize('25', hoursUnit)).toBe(false);
+        expect(validateWindowSize('1', hoursUnit)).toBe(true);
+        expect(validateWindowSize('24', hoursUnit)).toBe(true);
+      });
+    });
+
+    describe('validateDeleteAfterDays', () => {
+      it('should validate local index correctly', () => {
+        const localIndexType = EXPORTER_TYPE.localIndex;
+        expect(validateDeleteAfterDays('0', localIndexType)).toBe(false);
+        expect(validateDeleteAfterDays('181', localIndexType)).toBe(false);
+        expect(validateDeleteAfterDays('1', localIndexType)).toBe(true);
+        expect(validateDeleteAfterDays('180', localIndexType)).toBe(true);
+      });
+
+      it('should return true for non-local index', () => {
+        const noneType = EXPORTER_TYPE.none;
+        expect(validateDeleteAfterDays('0', noneType)).toBe(true);
+        expect(validateDeleteAfterDays('abc', noneType)).toBe(true);
+      });
+    });
+
+    describe('validateConfiguration', () => {
+      it('should return false when any validation fails', () => {
+        expect(validateConfiguration('101', '5', 'MINUTES', '30', EXPORTER_TYPE.localIndex)).toBe(
+          false
+        );
+        expect(validateConfiguration('50', '', 'MINUTES', '30', EXPORTER_TYPE.localIndex)).toBe(
+          false
+        );
+        expect(validateConfiguration('50', '5', 'MINUTES', '181', EXPORTER_TYPE.localIndex)).toBe(
+          false
+        );
+      });
+    });
+  });
 });

public/pages/Configuration/Configuration.tsx

@@ -43,6 +43,7 @@ import {
 } from '../../../common/constants';
 import { QueryInsightsDataSourceMenu } from '../../components/DataSourcePicker';
 import { QueryInsightsDashboardsPluginStartDependencies } from '../../types';
+import { validateConfiguration } from './configurationValidation';
 
 const Configuration = ({
   latencySettings,
@@ -198,6 +199,9 @@ const Configuration = ({
   );
 
   const WindowChoice = time === TIME_UNITS_TEXT[0].value ? MinutesBox : HoursBox;
+  const isLocalIndex = exporterType === EXPORTER_TYPE.localIndex;
+  const parsedDeleteAfter = parseInt(deleteAfterDays, 10);
+  const isDeleteAfterValid = !isLocalIndex || (parsedDeleteAfter >= 1 && parsedDeleteAfter <= 180);
 
   const isChanged =
     isEnabled !== metricSettingsMap[metric].isEnabled ||
@@ -208,13 +212,7 @@ const Configuration = ({
     exporterType !== dataRetentionSettingMap.dataRetention.exporterType ||
     deleteAfterDays !== dataRetentionSettingMap.dataRetention.deleteAfterDays;
 
-  const isValid = (() => {
-    const nVal = parseInt(topNSize, 10);
-    if (nVal < 1 || nVal > 100) return false;
-    if (time === TIME_UNITS_TEXT[0].value) return true;
-    const windowVal = parseInt(windowSize, 10);
-    return windowVal >= 1 && windowVal <= 24;
-  })();
+  const isValid = validateConfiguration(topNSize, windowSize, time, deleteAfterDays, exporterType);
 
   const formRowPadding = { padding: '0px 0px 20px' };
   const enabledSymb = <EuiHealth color="primary">Enabled</EuiHealth>;
@@ -505,13 +503,25 @@ const Configuration = ({
                     />
                   </EuiFlexItem>
                   <EuiFlexItem>
-                    <EuiFormRow style={formRowPadding}>
+                    <EuiFormRow
+                      style={formRowPadding}
+                      helpText="Max allowed limit 180."
+                      isInvalid={isLocalIndex && !isDeleteAfterValid}
+                      error={
+                        isLocalIndex && !isDeleteAfterValid
+                          ? 'Please enter a value between 1 and 180.'
+                          : undefined
+                      }
+                    >
                       <EuiFieldNumber
-                        disabled={exporterType !== EXPORTER_TYPE.localIndex}
+                        disabled={!isLocalIndex}
                         min={1}
                         max={180}
-                        value={exporterType !== EXPORTER_TYPE.localIndex ? '' : deleteAfterDays}
+                        value={
+                          !isLocalIndex ? '' : deleteAfterDays === '' ? '' : Number(deleteAfterDays)
+                        }
                         onChange={onDeleteAfterDaysChange}
+                        isInvalid={isLocalIndex && !isDeleteAfterValid}
                       />
                     </EuiFormRow>
                   </EuiFlexItem>

public/pages/Configuration/__snapshots__/Configuration.test.tsx.snap

@@ -962,6 +962,7 @@ exports[`Configuration Component renders with default settings: should match def
                           class="euiFormControlLayout__childrenWrapper"
                         >
                           <input
+                            aria-describedby="random_html_id-help-0"
                             class="euiFieldNumber"
                             id="random_html_id"
                             max="180"
@@ -971,6 +972,12 @@ exports[`Configuration Component renders with default settings: should match def
                           />
                         </div>
                       </div>
+                      <div
+                        class="euiFormHelpText euiFormRow__text"
+                        id="random_html_id-help-0"
+                      >
+                        Max allowed limit 180.
+                      </div>
                     </div>
                   </div>
                 </div>
@@ -1991,6 +1998,7 @@ exports[`Configuration Component updates state when toggling metrics and enables
                           class="euiFormControlLayout__childrenWrapper"
                         >
                           <input
+                            aria-describedby="random_html_id-help-0"
                             class="euiFieldNumber"
                             id="random_html_id"
                             max="180"
@@ -2000,6 +2008,12 @@ exports[`Configuration Component updates state when toggling metrics and enables
                           />
                         </div>
                       </div>
+                      <div
+                        class="euiFormHelpText euiFormRow__text"
+                        id="random_html_id-help-0"
+                      >
+                        Max allowed limit 180.
+                      </div>
                     </div>
                   </div>
                 </div>

public/pages/Configuration/configurationValidation.ts

@@ -0,0 +1,55 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import { TIME_UNITS_TEXT, EXPORTER_TYPE, VALIDATION_LIMITS } from '../../../common/constants';
+
+export const validateTopNSize = (topNSize: string): boolean => {
+  const nVal = parseInt(topNSize, 10);
+  if (topNSize !== nVal.toString() || Number.isNaN(nVal)) return false;
+  return nVal >= VALIDATION_LIMITS.TOP_N_SIZE.MIN && nVal <= VALIDATION_LIMITS.TOP_N_SIZE.MAX;
+};
+
+export const validateWindowSize = (windowSize: string, timeUnit: string): boolean => {
+  if (timeUnit === TIME_UNITS_TEXT[0].value) {
+    // MINUTES
+    const windowVal = parseInt(windowSize, 10);
+    return windowSize !== '' && !Number.isNaN(windowVal) && windowSize === windowVal.toString();
+  } else {
+    // HOURS
+    const windowVal = parseInt(windowSize, 10);
+    if (windowSize !== windowVal.toString() || Number.isNaN(windowVal)) return false;
+    return (
+      windowVal >= VALIDATION_LIMITS.WINDOW_SIZE_HOURS.MIN &&
+      windowVal <= VALIDATION_LIMITS.WINDOW_SIZE_HOURS.MAX
+    );
+  }
+};
+
+export const validateDeleteAfterDays = (deleteAfterDays: string, exporterType: string): boolean => {
+  const isLocalIndex = exporterType === EXPORTER_TYPE.localIndex;
+  if (!isLocalIndex) return true;
+
+  const parsedDeleteAfter = parseInt(deleteAfterDays, 10);
+  if (deleteAfterDays !== parsedDeleteAfter.toString() || Number.isNaN(parsedDeleteAfter))
+    return false;
+  return (
+    parsedDeleteAfter >= VALIDATION_LIMITS.DELETE_AFTER_DAYS.MIN &&
+    parsedDeleteAfter <= VALIDATION_LIMITS.DELETE_AFTER_DAYS.MAX
+  );
+};
+
+export const validateConfiguration = (
+  topNSize: string,
+  windowSize: string,
+  timeUnit: string,
+  deleteAfterDays: string,
+  exporterType: string
+): boolean => {
+  return (
+    validateTopNSize(topNSize) &&
+    validateWindowSize(windowSize, timeUnit) &&
+    validateDeleteAfterDays(deleteAfterDays, exporterType)
+  );
+};