aminghadersohi commented on code in PR #44237:
URL: https://github.com/apache/superset/pull/44237#discussion_r4009583019
##########
superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx:
##########
@@ -165,6 +166,10 @@ export default function DateFilterLabel(props:
DateFilterControlProps) {
const [isDescriptionHovered, setIsDescriptionHovered] = useState(false);
const theme = useTheme();
const [labelRef, labelIsTruncated] = useCSSTextTruncation<HTMLSpanElement>();
+ // Shared across both fetches below since they can write overlapping state
+ // (evalResponse, validTimeRange, lastFetchedTimeRange); guards against an
+ // older, slower request overwriting a newer one that resolved first.
+ const latestRequestId = useRef(0);
Review Comment:
Both effects share this counter, so the debounced draft fetch cancels effect
1's — and effect 1 is the only writer of `actualTimeRange`/`tooltipTitle`, with
no retry. Apply within the 500 ms debounce and the pill keeps the old range;
pre-PR it updates. Needs one ref per effect.
##########
superset-frontend/src/filters/components/Time/TimeFilterPlugin.tsx:
##########
@@ -131,6 +131,7 @@ export default function TimeFilterPlugin(props:
PluginFilterTimeProps) {
unsetFocusedFilter();
}}
isOverflowingFilterBar={isOverflowingFilterBar}
+ displayFormat={props.formData.displayFormat}
Review Comment:
Deleting this line makes the feature a no-op, yet all 171 tests under
`FiltersConfigModal/` and `filters/components/Time/` still pass — likewise for
dropping the `displayFormat` exclusion in `getControlItemsMap`. The
controlValues→formData→control wiring is unpinned.
##########
superset-frontend/packages/superset-ui-core/test/time-comparison/fetchTimeRange.test.ts:
##########
@@ -58,6 +58,41 @@ test('generates a readable time range', () => {
expect(formatTimeRange('')).toBe('');
});
+test('formatTimeRange applies an optional D3 date format to both endpoints
without reinterpreting the timezone', () => {
+ // jest.config.js pins the test runner to America/New_York, so if the
+ // implementation ever parsed these naive ISO strings as local time
+ // instead of UTC, the formatted date/time below would shift.
+ expect(
+ formatTimeRange(
+ '2019-01-14T23:30:00 : 2019-01-21T08:15:00',
+ 'col',
+ '%Y-%m-%d %H:%M',
+ ),
+ ).toBe('2019-01-14 23:30 ≤ col < 2019-01-21 08:15');
+
+ expect(
+ formatTimeRange(
+ '2019-01-14T01:32:10 : 2019-01-21T01:32:10',
+ 'col',
+ '%d-%m-%Y %H:%M:%S',
+ ),
+ ).toBe('14-01-2019 01:32:10 ≤ col < 21-01-2019 01:32:10');
+});
+
+test('formatTimeRange preserves the -∞/∞ placeholders when a date format is
set', () => {
+ expect(formatTimeRange('2019-01-14T00:00:00 : ', 'col', '%d-%m-%Y')).toBe(
+ '14-01-2019 ≤ col < ∞',
+ );
+ expect(formatTimeRange(' : 2019-01-21T00:00:00', 'col', '%d-%m-%Y')).toBe(
+ '-∞ ≤ col < 21-01-2019',
+ );
+});
+
+test('formatTimeRange leaves human-readable values untouched even when a date
format is set', () => {
+ expect(formatTimeRange('Last week', 'col', '%d-%m-%Y')).toBe('Last week');
+ expect(formatTimeRange('No filter', 'col', '%d-%m-%Y')).toBe('No filter');
Review Comment:
`'Last week'`/`'No filter'` have no ` : ` separator, so they return before
`formatDateEndpoint` — this passes unchanged on master. Swapping `isValid()`
for `true` leaves all 20 tests green while `'Yesterday : Tomorrow'` renders
`Invalid Date ≤ col < Invalid Date`.
```suggestion
expect(formatTimeRange('Last week', 'col', '%d-%m-%Y')).toBe('Last week');
expect(formatTimeRange('No filter', 'col', '%d-%m-%Y')).toBe('No filter');
expect(formatTimeRange('Yesterday : Tomorrow', 'col', '%d-%m-%Y')).toBe(
'Yesterday ≤ col < Tomorrow',
);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
