Implement aggregate operations on dates #9444

ijreilly · 2025-01-07T18:01:08Z

Adding aggregate operations for dates: min ("Earliest date") and max ("Latest date")

greptile-apps

PR Summary

This PR implements date aggregation operations (min/max) across the record board and table components, allowing users to find earliest and latest dates in datasets.

Added new ExtendedAggregateOperations type with 'EARLIEST'/'LATEST' operations in /packages/twenty-front/src/modules/object-record/record-table/types/ExtendedAggregateOperations.ts
Implemented date formatting with user preferences in /packages/twenty-front/src/utils/string/formatDateString.ts for consistent date display
Added DateTime field support for min/max operations in /packages/twenty-front/src/modules/object-record/record-table/constants/FieldTypesAvailableForNonStandardAggregateOperation.ts
Created conversion utilities between standard and extended operations in /packages/twenty-front/src/modules/object-record/utils/convertAggregateOperationToExtendedAggregateOperation.ts
Updated server-side descriptions from 'Oldest/Most recent' to 'Earliest/Latest' date in /packages/twenty-server/src/engine/api/graphql/workspace-schema-builder/utils/get-available-aggregations-from-object-fields.util.ts

_{28 file(s) reviewed, 17 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-01-08T10:36:42Z

...t-record/record-board/record-board-column/utils/__tests__/getAggregateOperationLabel.test.ts

+    expect(getAggregateOperationLabel('EARLIEST')).toBe('Earliest date');
+    expect(getAggregateOperationLabel('LATEST')).toBe('Latest date');


style: Testing new date aggregate operations 'EARLIEST' and 'LATEST' which are not defined in AGGREGATE_OPERATIONS enum. Consider adding these to the enum for consistency.

greptile-apps · 2025-01-08T10:36:57Z

...c/modules/object-record/record-board/record-board-column/utils/getAggregateOperationLabel.ts

+    case 'EARLIEST':
+      return 'Earliest date';
+    case 'LATEST':
+      return 'Latest date';


style: Consider using AGGREGATE_OPERATIONS enum for EARLIEST/LATEST instead of string literals to maintain consistency with other cases

greptile-apps · 2025-01-08T10:37:01Z

...odules/object-record/record-board/record-board-column/utils/computeAggregateValueAndLabel.ts

        const { decimals, type } = field.settings ?? {};
        value =
          type === 'percentage'
            ? `${formatNumber(value * 100, decimals)}%`
            : formatNumber(value, decimals);
        break;
      }
+
+      case FieldMetadataType.DateTime: {
+        value = aggregateValue as string;


logic: type assertion to string could be unsafe if aggregateValue is not actually a string

greptile-apps · 2025-01-08T10:37:02Z

...odules/object-record/record-board/record-board-column/utils/computeAggregateValueAndLabel.ts

+        value = formatDateString({
+          value,
+          displayAsRelativeDate,
+          timeZone,
+          dateFormat,
+          timeFormat,
+        });


style: formatDateString could throw if value is invalid - consider adding error handling

greptile-apps · 2025-01-08T10:38:34Z

...ges/twenty-front/src/modules/object-record/record-table/types/ExtendedAggregateOperations.ts

+  | 'EARLIEST'
+  | 'LATEST';


style: consider using an enum for these values to maintain consistency with AGGREGATE_OPERATIONS and prevent string literal typos

greptile-apps · 2025-01-08T10:40:40Z

packages/twenty-front/src/utils/string/__tests__/formatDateString.test.ts

+    jest.mock('@/localization/utils/formatDateISOStringToDateTime', () => ({
+      formatDateISOStringToDateTime: jest
+        .fn()
+        .mockReturnValue(mockFormattedDate),
+    }));


style: Duplicate mock implementation across multiple tests. Consider using beforeEach() to set up mocks and afterEach() to clear them.

greptile-apps · 2025-01-08T10:40:40Z

packages/twenty-front/src/utils/string/__tests__/formatDateString.test.ts

+  it('should format date as relative when displayAsRelativeDate is true', () => {
+    const mockDate = DateTime.now().minus({ months: 2 }).toISO();
+    const mockRelativeDate = '2 months ago';


logic: Using DateTime.now() in tests can make them non-deterministic. Use a fixed date instead.

greptile-apps · 2025-01-08T10:43:02Z

packages/twenty-front/src/utils/string/formatDateString.ts

+  const formattedDate = value
+    ? displayAsRelativeDate
+      ? formatDateISOStringToRelativeDate(value)
+      : formatDateISOStringToDateTime(value, timeZone, dateFormat, timeFormat)
+    : '';


style: consider adding input validation for value format before passing to formatDateISOStringToDateTime/RelativeDate to prevent runtime errors with malformed dates

greptile-apps · 2025-01-08T10:43:03Z

packages/twenty-front/src/utils/string/formatDateString.ts

+  timeZone: string;
+  dateFormat: DateFormat;
+  timeFormat: TimeFormat;
+  value?: string | null;
+  displayAsRelativeDate?: boolean;


style: all parameters except value and displayAsRelativeDate are required but lack validation - consider adding runtime checks

packages/twenty-front/src/utils/string/formatDateString.ts

Weiko

LGTM!

Weiko · 2025-01-08T10:43:19Z

...ont/src/modules/object-record/utils/convertExtendedAggregateOperationToAggregateOperation.ts

+
+import { ExtendedAggregateOperations } from '@/object-record/record-table/types/ExtendedAggregateOperations';
+
+export const convertExtendedAggregateOperationToAggregateOperation = (


"Extended" seems very broad. Looks like we are talking about "dates" here right? Why do we need to introduce this and not simply use min for dates? Is it for display?

I'm not sure what's the best strategy here actually, let's keep it like that until we have other things to convert, it's not clear to me 🙂

FYI you can also use map for this kind of thing, might be easier to maintain?

const dateTimeOperationMap: Record<AGGREGATE_OPERATIONS, ExtendedAggregateOperations> = { [AGGREGATE_OPERATIONS.min]: 'EARLIEST', [AGGREGATE_OPERATIONS.max]: 'LATEST',

then in your code you can just use the map based on its field metadata type (via factories? But I think it's not a practice in the FE though)
Anyway, just FYI!

Actually I need to be able to "convert" in both senses. I think there is no perfect solution for this, I will also need two maps, or to use the map using values as keys sometimes :/

Weiko · 2025-01-08T10:51:03Z

...front/src/modules/object-record/utils/getAvailableFieldsIdsForAggregationFromObjectFields.ts

+          if (acc[convertedAggregateOperation] === undefined) {
+            acc[convertedAggregateOperation] = [];
+          }
+          acc[convertedAggregateOperation]?.push(field.id);


Suggested change

acc[convertedAggregateOperation]?.push(field.id);

acc[convertedAggregateOperation].push(field.id);

looks like you don't need "?" anymore here

Weiko · 2025-01-08T10:51:39Z

...y-front/src/modules/object-record/utils/initializeAvailableFieldsForAggregateOperationMap.ts

@@ -8,6 +9,7 @@ export const initializeAvailableFieldsForAggregateOperationMap = (
    (acc, operation) => ({
      ...acc,
      [operation]: [],


shouldn't we remove this line?
In practice it will try to do

{ min: [], min: [], max: [], max: [], earliest: [], latest: [], }

but js will actually create

{ min: [], max: [], earliest: [], latest: [], }

so it does not matter much but let's remove the line if it's not necessary!

Weiko · 2025-01-08T10:56:20Z

...record/record-table/record-table-footer/components/RecordTableColumnAggregateFooterValue.tsx

  gap: 4px;
  height: 32px;
  justify-content: flex-end;
  padding: 0 8px;
+  overflow-x: auto;


nit: not really sure as I don't do much FE but it seems you are adding the same css to both styled components, do we use some kind of shared style components in the codebase for that?

I did not find any shared component but I extracted the style to use it for both components

ijreilly added 3 commits January 7, 2025 18:45

Implement aggregate operations on dates

9c80b1c

Fix ui and test

155814a

Add tests for formatDateString

3247625

ijreilly marked this pull request as ready for review January 8, 2025 10:35

charlesBochet assigned Weiko and ijreilly Jan 8, 2025

charlesBochet added the -PR: awaiting review label Jan 8, 2025

greptile-apps bot reviewed Jan 8, 2025

View reviewed changes

Weiko approved these changes Jan 8, 2025

View reviewed changes

charlesBochet added -PR: awaiting author and removed -PR: awaiting review labels Jan 8, 2025

ijreilly added 2 commits January 8, 2025 16:21

minor fixes

4d8b019

fix typecheck

e22f407

ijreilly enabled auto-merge (squash) January 8, 2025 15:36

ijreilly merged commit 8475b55 into main Jan 8, 2025
23 checks passed

ijreilly deleted the aggregate-dates branch January 8, 2025 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement aggregate operations on dates #9444

Implement aggregate operations on dates #9444

ijreilly commented Jan 7, 2025 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Jan 8, 2025

greptile-apps bot Jan 8, 2025

greptile-apps bot Jan 8, 2025

greptile-apps bot Jan 8, 2025

greptile-apps bot Jan 8, 2025

greptile-apps bot Jan 8, 2025

greptile-apps bot Jan 8, 2025

greptile-apps bot Jan 8, 2025

greptile-apps bot Jan 8, 2025

Weiko left a comment

Weiko Jan 8, 2025

Weiko Jan 8, 2025

Weiko Jan 8, 2025

ijreilly Jan 8, 2025

Weiko Jan 8, 2025

Weiko Jan 8, 2025

Weiko Jan 8, 2025

ijreilly Jan 8, 2025

		expect(getAggregateOperationLabel('EARLIEST')).toBe('Earliest date');
		expect(getAggregateOperationLabel('LATEST')).toBe('Latest date');


		import { ExtendedAggregateOperations } from '@/object-record/record-table/types/ExtendedAggregateOperations';

		export const convertExtendedAggregateOperationToAggregateOperation = (

	acc[convertedAggregateOperation]?.push(field.id);
	acc[convertedAggregateOperation].push(field.id);

Implement aggregate operations on dates #9444

Implement aggregate operations on dates #9444

Conversation

ijreilly commented Jan 7, 2025 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jan 8, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 8, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 8, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 8, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 8, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 8, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 8, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 8, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 8, 2025

Choose a reason for hiding this comment

Weiko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijreilly commented Jan 7, 2025 •

edited

Loading