[v9.3.x] TimeRangePicker: Fix recent ranges not showing all items (#60085)

TimeRangePicker: Fix recent ranges not showing all items (#59836)

* Fix not all recently used time ranges showing

* Refactor time picker history to store simpler json objects

* Don't store duplicate items

* update todo tests:

* Add tests for TimePickerWithHistory

* better fix for focus scope issues in test

(cherry picked from commit 6280780e0c)

Co-authored-by: Josh Hunt <joshhunt@users.noreply.github.com>

packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker.tsx

@@ -122,8 +122,8 @@ export function TimeRangePicker(props: TimeRangePickerProps) {
         </ToolbarButton>
       </Tooltip>
       {isOpen && (
-        <FocusScope contain autoFocus>
-          <section ref={ref} {...overlayProps} {...dialogProps}>
+        <section ref={ref} {...overlayProps} {...dialogProps}>
+          <FocusScope contain autoFocus>
             <TimePickerContent
               timeZone={timeZone}
               fiscalYearStartMonth={fiscalYearStartMonth}
@@ -137,8 +137,8 @@ export function TimeRangePicker(props: TimeRangePickerProps) {
               onChangeFiscalYearStartMonth={onChangeFiscalYearStartMonth}
               hideQuickRanges={hideQuickRanges}
             />
-          </section>
-        </FocusScope>
+          </FocusScope>
+        </section>
       )}
 
       {timeSyncButton}

packages/grafana-ui/src/components/DateTimePickers/TimeRangePicker/TimePickerContent.tsx

@@ -160,7 +160,6 @@ const NarrowScreenForm = (props: FormProps) => {
           <div className={styles.form}>
             <TimeRangeContent value={value} onApply={onChange} timeZone={timeZone} isFullscreen={false} />
           </div>
-          <p></p>
           {showHistory && (
             <TimeRangeList
               title={t('time-picker.absolute.recent-title', 'Recently used absolute ranges')}
@@ -246,7 +245,8 @@ function mapToHistoryOptions(ranges?: TimeRange[], timeZone?: TimeZone): TimeOpt
   if (!Array.isArray(ranges) || ranges.length === 0) {
     return [];
   }
-  return ranges.slice(ranges.length - 4).map((range) => mapRangeToTimeOption(range, timeZone));
+
+  return ranges.map((range) => mapRangeToTimeOption(range, timeZone));
 }
 
 EmptyRecentList.displayName = 'EmptyRecentList';

packages/grafana-ui/src/components/Dropdown/ButtonSelect.test.tsx

@@ -0,0 +1,56 @@
+import { render, screen } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import React from 'react';
+
+import { SelectableValue } from '@grafana/data';
+
+import { ButtonSelect } from './ButtonSelect';
+
+const OPTIONS: SelectableValue[] = [
+  {
+    label: 'Hello',
+    value: 'a',
+  },
+  {
+    label: 'World',
+    value: 'b',
+  },
+];
+
+describe('ButtonSelect', () => {
+  it('initially renders the selected value with the menu closed', () => {
+    const selected = OPTIONS[0];
+    render(<ButtonSelect value={selected} options={OPTIONS} onChange={() => {}} />);
+
+    expect(screen.getByText('Hello')).toBeInTheDocument();
+    expect(screen.queryAllByRole('menuitemradio')).toHaveLength(0);
+  });
+
+  it('opens the menu when clicking the button', async () => {
+    const selected = OPTIONS[0];
+    render(<ButtonSelect value={selected} options={OPTIONS} onChange={() => {}} />);
+
+    const button = screen.getByText('Hello');
+    await userEvent.click(button);
+
+    expect(screen.queryAllByRole('menuitemradio')).toHaveLength(2);
+  });
+
+  it('closes the menu when clicking an option', async () => {
+    const selected = OPTIONS[0];
+    const onChange = jest.fn();
+    render(<ButtonSelect value={selected} options={OPTIONS} onChange={onChange} />);
+
+    const button = screen.getByText('Hello');
+    await userEvent.click(button);
+
+    const option = screen.getByText('World');
+    await userEvent.click(option);
+
+    expect(screen.queryAllByRole('menuitemradio')).toHaveLength(0);
+    expect(onChange).toHaveBeenCalledWith({
+      label: 'World',
+      value: 'b',
+    });
+  });
+});

public/app/core/components/TimePicker/TimePickerWithHistory.test.tsx

@@ -0,0 +1,132 @@
+import { render, screen } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import React from 'react';
+
+import { getDefaultTimeRange } from '@grafana/data';
+
+import { TimePickerWithHistory } from './TimePickerWithHistory';
+
+describe('TimePickerWithHistory', () => {
+  // In some of the tests we close and re-open the picker. When we do that we must re-find these inputs
+  // as new elements will have been mounted
+  const getFromField = () => screen.getByLabelText('Time Range from field');
+  const getToField = () => screen.getByLabelText('Time Range to field');
+  const getApplyButton = () => screen.getByRole('button', { name: 'Apply time range' });
+
+  const LOCAL_STORAGE_KEY = 'grafana.dashboard.timepicker.history';
+  const OLD_LOCAL_STORAGE = [
+    {
+      from: '2022-12-03T00:00:00.000Z',
+      to: '2022-12-03T23:59:59.000Z',
+      raw: { from: '2022-12-03T00:00:00.000Z', to: '2022-12-03T23:59:59.000Z' },
+    },
+    {
+      from: '2022-12-02T00:00:00.000Z',
+      to: '2022-12-02T23:59:59.000Z',
+      raw: { from: '2022-12-02T00:00:00.000Z', to: '2022-12-02T23:59:59.000Z' },
+    },
+  ];
+
+  const NEW_LOCAL_STORAGE = [
+    { from: '2022-12-03T00:00:00.000Z', to: '2022-12-03T23:59:59.000Z' },
+    { from: '2022-12-02T00:00:00.000Z', to: '2022-12-02T23:59:59.000Z' },
+  ];
+
+  const props = {
+    timeZone: 'utc',
+    onChange: () => {},
+    onChangeTimeZone: () => {},
+    onMoveBackward: () => {},
+    onMoveForward: () => {},
+    onZoom: () => {},
+  };
+
+  afterEach(() => window.localStorage.clear());
+
+  it('Should load with no history', async () => {
+    const timeRange = getDefaultTimeRange();
+    render(<TimePickerWithHistory value={timeRange} {...props} />);
+    await userEvent.click(screen.getByLabelText(/Time range selected/));
+
+    expect(screen.getByText(/It looks like you haven't used this time picker before/i)).toBeInTheDocument();
+  });
+
+  it('Should load with old TimeRange history', async () => {
+    window.localStorage.setItem(LOCAL_STORAGE_KEY, JSON.stringify(OLD_LOCAL_STORAGE));
+
+    const timeRange = getDefaultTimeRange();
+    render(<TimePickerWithHistory value={timeRange} {...props} />);
+    await userEvent.click(screen.getByLabelText(/Time range selected/));
+
+    expect(screen.getByText(/2022-12-03 00:00:00 to 2022-12-03 23:59:59/i)).toBeInTheDocument();
+    expect(screen.queryByText(/2022-12-02 00:00:00 to 2022-12-02 23:59:59/i)).toBeInTheDocument();
+  });
+
+  it('Should load with new TimePickerHistoryItem history', async () => {
+    window.localStorage.setItem(LOCAL_STORAGE_KEY, JSON.stringify(NEW_LOCAL_STORAGE));
+
+    const timeRange = getDefaultTimeRange();
+    render(<TimePickerWithHistory value={timeRange} {...props} />);
+    await userEvent.click(screen.getByLabelText(/Time range selected/));
+
+    expect(screen.queryByText(/2022-12-03 00:00:00 to 2022-12-03 23:59:59/i)).toBeInTheDocument();
+    expect(screen.queryByText(/2022-12-02 00:00:00 to 2022-12-02 23:59:59/i)).toBeInTheDocument();
+  });
+
+  it('Saves changes into local storage without duplicates', async () => {
+    const timeRange = getDefaultTimeRange();
+    render(<TimePickerWithHistory value={timeRange} {...props} />);
+    await userEvent.click(screen.getByLabelText(/Time range selected/));
+
+    await clearAndType(getFromField(), '2022-12-03 00:00:00');
+    await clearAndType(getToField(), '2022-12-03 23:59:59');
+    await userEvent.click(getApplyButton());
+
+    await userEvent.click(screen.getByLabelText(/Time range selected/));
+
+    // Same range again!
+    await clearAndType(getFromField(), '2022-12-03 00:00:00');
+    await clearAndType(getToField(), '2022-12-03 23:59:59');
+    await userEvent.click(getApplyButton());
+
+    const newLsValue = JSON.parse(window.localStorage.getItem(LOCAL_STORAGE_KEY) ?? '[]');
+    expect(newLsValue).toEqual([{ from: '2022-12-03T00:00:00.000Z', to: '2022-12-03T23:59:59.000Z' }]);
+  });
+
+  it('Should show 4 most recently used time ranges', async () => {
+    const inputRanges: Array<[string, string]> = [
+      ['2022-12-10 00:00:00', '2022-12-10 23:59:59'],
+      ['2022-12-11 00:00:00', '2022-12-11 23:59:59'],
+      ['2022-12-12 00:00:00', '2022-12-12 23:59:59'],
+      ['2022-12-13 00:00:00', '2022-12-13 23:59:59'],
+      ['2022-12-14 00:00:00', '2022-12-14 23:59:59'],
+    ];
+
+    const expectedLocalStorage = [
+      { from: '2022-12-14T00:00:00.000Z', to: '2022-12-14T23:59:59.000Z' },
+      { from: '2022-12-13T00:00:00.000Z', to: '2022-12-13T23:59:59.000Z' },
+      { from: '2022-12-12T00:00:00.000Z', to: '2022-12-12T23:59:59.000Z' },
+      { from: '2022-12-11T00:00:00.000Z', to: '2022-12-11T23:59:59.000Z' },
+    ];
+
+    const timeRange = getDefaultTimeRange();
+    render(<TimePickerWithHistory value={timeRange} {...props} />);
+    await userEvent.click(screen.getByLabelText(/Time range selected/));
+
+    for (const [inputFrom, inputTo] of inputRanges) {
+      await userEvent.click(screen.getByLabelText(/Time range selected/));
+      await clearAndType(getFromField(), inputFrom);
+      await clearAndType(getToField(), inputTo);
+
+      await userEvent.click(getApplyButton());
+    }
+
+    const newLsValue = JSON.parse(window.localStorage.getItem(LOCAL_STORAGE_KEY) ?? '[]');
+    expect(newLsValue).toEqual(expectedLocalStorage);
+  });
+});
+
+async function clearAndType(field: HTMLElement, text: string) {
+  await userEvent.clear(field);
+  return await userEvent.type(field, text);
+}

public/app/core/components/TimePicker/TimePickerWithHistory.tsx

@@ -1,6 +1,7 @@
+import { uniqBy } from 'lodash';
 import React from 'react';
 
-import { TimeRange, isDateTime, toUtc } from '@grafana/data';
+import { TimeRange, isDateTime, rangeUtil, TimeZone } from '@grafana/data';
 import { TimeRangePickerProps, TimeRangePicker } from '@grafana/ui';
 
 import { LocalStorageValueProvider } from '../LocalStorageValueProvider';
@@ -9,14 +10,26 @@ const LOCAL_STORAGE_KEY = 'grafana.dashboard.timepicker.history';
 
 interface Props extends Omit<TimeRangePickerProps, 'history' | 'theme'> {}
 
+// Simplified object to store in local storage
+interface TimePickerHistoryItem {
+  from: string;
+  to: string;
+}
+
+// We should only be storing TimePickerHistoryItem, but in the past we also stored TimeRange
+type LSTimePickerHistoryItem = TimePickerHistoryItem | TimeRange;
+
 export const TimePickerWithHistory = (props: Props) => {
   return (
-    <LocalStorageValueProvider<TimeRange[]> storageKey={LOCAL_STORAGE_KEY} defaultValue={[]}>
-      {(values, onSaveToStore) => {
+    <LocalStorageValueProvider<LSTimePickerHistoryItem[]> storageKey={LOCAL_STORAGE_KEY} defaultValue={[]}>
+      {(rawValues, onSaveToStore) => {
+        const values = migrateHistory(rawValues);
+        const history = deserializeHistory(values, props.timeZone);
+
         return (
           <TimeRangePicker
             {...props}
-            history={convertIfJson(values)}
+            history={history}
             onChange={(value) => {
               onAppendToHistory(value, values, onSaveToStore);
               props.onChange(value);
@@ -28,24 +41,37 @@ export const TimePickerWithHistory = (props: Props) => {
   );
 };
 
-function convertIfJson(history: TimeRange[]): TimeRange[] {
-  return history.map((time) => {
-    if (isDateTime(time.from)) {
-      return time;
-    }
+function deserializeHistory(values: TimePickerHistoryItem[], timeZone: TimeZone | undefined): TimeRange[] {
+  return values.map((item) => rangeUtil.convertRawToRange(item, timeZone));
+}
+
+function migrateHistory(values: LSTimePickerHistoryItem[]): TimePickerHistoryItem[] {
+  return values.map((item) => {
+    const fromValue = typeof item.from === 'string' ? item.from : item.from.toISOString();
+    const toValue = typeof item.to === 'string' ? item.to : item.to.toISOString();
 
     return {
-      from: toUtc(time.from),
-      to: toUtc(time.to),
-      raw: time.raw,
+      from: fromValue,
+      to: toValue,
     };
   });
 }
 
-function onAppendToHistory(toAppend: TimeRange, values: TimeRange[], onSaveToStore: (values: TimeRange[]) => void) {
-  if (!isAbsolute(toAppend)) {
+function onAppendToHistory(
+  newTimeRange: TimeRange,
+  values: TimePickerHistoryItem[],
+  onSaveToStore: (values: TimePickerHistoryItem[]) => void
+) {
+  if (!isAbsolute(newTimeRange)) {
     return;
   }
+
+  // Convert DateTime objects to strings
+  const toAppend = {
+    from: typeof newTimeRange.raw.from === 'string' ? newTimeRange.raw.from : newTimeRange.raw.from.toISOString(),
+    to: typeof newTimeRange.raw.to === 'string' ? newTimeRange.raw.to : newTimeRange.raw.to.toISOString(),
+  };
+
   const toStore = limit([toAppend, ...values]);
   onSaveToStore(toStore);
 }
@@ -54,6 +80,6 @@ function isAbsolute(value: TimeRange): boolean {
   return isDateTime(value.raw.from) || isDateTime(value.raw.to);
 }
 
-function limit(value: TimeRange[]): TimeRange[] {
-  return value.slice(0, 4);
+function limit(value: TimePickerHistoryItem[]): TimePickerHistoryItem[] {
+  return uniqBy(value, (v) => v.from + v.to).slice(0, 4);
 }