Skip to content

Commit 5200042

Browse files
msyavuzmsyavuz
committed
fix: attend reviews
1 parent 372397c commit 5200042

File tree

2 files changed

+149
-140
lines changed

2 files changed

+149
-140
lines changed

‎superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -56,12 +56,12 @@ export const getSmartDateFormatter = (timeGrain?: string) => {
5656
normalizedDate.setMilliseconds(0);
5757

5858
// For all time grains, normalize using UTC methods to avoid timezone issues
59-
if (timeGrain === TimeGranularity.YEAR || timeGrain === 'P1Y') {
59+
if (timeGrain === TimeGranularity.YEAR) {
6060
// Set to January 1st at midnight UTC - smart formatter will show year
6161
const year = normalizedDate.getUTCFullYear();
6262
const cleanDate = new Date(Date.UTC(year, 0, 1, 0, 0, 0, 0));
6363
return baseFormatter(cleanDate);
64-
} else if (timeGrain === TimeGranularity.QUARTER || timeGrain === 'P3M') {
64+
} else if (timeGrain === TimeGranularity.QUARTER) {
6565
// Set to first month of quarter, first day, midnight UTC
6666
const year = normalizedDate.getUTCFullYear();
6767
const month = normalizedDate.getUTCMonth();
@@ -70,27 +70,27 @@ export const getSmartDateFormatter = (timeGrain?: string) => {
7070
Date.UTC(year, quarterStartMonth, 1, 0, 0, 0, 0),
7171
);
7272
return baseFormatter(cleanDate);
73-
} else if (timeGrain === TimeGranularity.MONTH || timeGrain === 'P1M') {
73+
} else if (timeGrain === TimeGranularity.MONTH) {
7474
// Set to first of month at midnight UTC - smart formatter will show month name or year
7575
const year = normalizedDate.getUTCFullYear();
7676
const month = normalizedDate.getUTCMonth();
7777
const cleanDate = new Date(Date.UTC(year, month, 1, 0, 0, 0, 0));
7878
return baseFormatter(cleanDate);
79-
} else if (timeGrain === TimeGranularity.WEEK || timeGrain === 'P1W') {
79+
} else if (timeGrain === TimeGranularity.WEEK) {
8080
// Set to midnight UTC, keep the day
8181
const year = normalizedDate.getUTCFullYear();
8282
const month = normalizedDate.getUTCMonth();
8383
const day = normalizedDate.getUTCDate();
8484
const cleanDate = new Date(Date.UTC(year, month, day, 0, 0, 0, 0));
8585
return baseFormatter(cleanDate);
86-
} else if (timeGrain === TimeGranularity.DAY || timeGrain === 'P1D') {
86+
} else if (timeGrain === TimeGranularity.DAY) {
8787
// Set to midnight UTC
8888
const year = normalizedDate.getUTCFullYear();
8989
const month = normalizedDate.getUTCMonth();
9090
const day = normalizedDate.getUTCDate();
9191
const cleanDate = new Date(Date.UTC(year, month, day, 0, 0, 0, 0));
9292
return baseFormatter(cleanDate);
93-
} else if (timeGrain === TimeGranularity.HOUR || timeGrain === 'PT1H') {
93+
} else if (timeGrain === TimeGranularity.HOUR) {
9494
// Set to top of hour UTC
9595
const year = normalizedDate.getUTCFullYear();
9696
const month = normalizedDate.getUTCMonth();

‎superset-frontend/plugins/plugin-chart-echarts/test/utils/formatters.test.ts‎

Lines changed: 143 additions & 134 deletions
Original file line numberDiff line numberDiff line change
@@ -16,157 +16,166 @@
1616
* specific language governing permissions and limitations
1717
* under the License.
1818
*/
19-
import { NumberFormats, SMART_DATE_ID, TimeFormatter } from '@superset-ui/core';
19+
import {
20+
NumberFormats,
21+
SMART_DATE_ID,
22+
TimeFormatter,
23+
TimeGranularity,
24+
} from '@superset-ui/core';
2025
import {
2126
getPercentFormatter,
2227
getXAxisFormatter,
2328
} from '../../src/utils/formatters';
2429

25-
describe('getPercentFormatter', () => {
30+
test('getPercentFormatter should format as percent if no format is specified', () => {
2631
const value = 0.6;
27-
test('should format as percent if no format is specified', () => {
28-
expect(getPercentFormatter().format(value)).toEqual('60%');
29-
});
30-
test('should format as percent if SMART_NUMBER is specified', () => {
31-
expect(
32-
getPercentFormatter(NumberFormats.SMART_NUMBER).format(value),
33-
).toEqual('60%');
34-
});
35-
test('should format using a provided format', () => {
36-
expect(
37-
getPercentFormatter(NumberFormats.PERCENT_2_POINT).format(value),
38-
).toEqual('60.00%');
39-
});
32+
expect(getPercentFormatter().format(value)).toEqual('60%');
4033
});
4134

42-
describe('getXAxisFormatter', () => {
43-
test('should return smart date formatter for SMART_DATE_ID format', () => {
44-
const formatter = getXAxisFormatter(SMART_DATE_ID);
45-
expect(formatter).toBeDefined();
46-
expect(formatter).toBeInstanceOf(TimeFormatter);
47-
expect((formatter as TimeFormatter).id).toBe(SMART_DATE_ID);
48-
});
35+
test('getPercentFormatter should format as percent if SMART_NUMBER is specified', () => {
36+
const value = 0.6;
37+
expect(getPercentFormatter(NumberFormats.SMART_NUMBER).format(value)).toEqual(
38+
'60%',
39+
);
40+
});
4941

50-
test('should return smart date formatter for undefined format', () => {
51-
const formatter = getXAxisFormatter();
52-
expect(formatter).toBeDefined();
53-
expect(formatter).toBeInstanceOf(TimeFormatter);
54-
expect((formatter as TimeFormatter).id).toBe(SMART_DATE_ID);
55-
});
42+
test('getPercentFormatter should format using a provided format', () => {
43+
const value = 0.6;
44+
expect(
45+
getPercentFormatter(NumberFormats.PERCENT_2_POINT).format(value),
46+
).toEqual('60.00%');
47+
});
5648

57-
test('should return custom time formatter for custom format', () => {
58-
const customFormat = '%Y-%m-%d';
59-
const formatter = getXAxisFormatter(customFormat);
60-
expect(formatter).toBeDefined();
61-
expect(formatter).toBeInstanceOf(TimeFormatter);
62-
expect((formatter as TimeFormatter).id).toBe(customFormat);
63-
});
49+
test('getXAxisFormatter should return smart date formatter for SMART_DATE_ID format', () => {
50+
const formatter = getXAxisFormatter(SMART_DATE_ID);
51+
expect(formatter).toBeDefined();
52+
expect(formatter).toBeInstanceOf(TimeFormatter);
53+
expect((formatter as TimeFormatter).id).toBe(SMART_DATE_ID);
54+
});
6455

65-
test('smart date formatter should be returned and not undefined', () => {
66-
const formatter = getXAxisFormatter(SMART_DATE_ID);
67-
expect(formatter).toBeDefined();
68-
expect(formatter).toBeInstanceOf(TimeFormatter);
69-
expect((formatter as TimeFormatter).id).toBe(SMART_DATE_ID);
70-
71-
const undefinedFormatter = getXAxisFormatter(undefined);
72-
expect(undefinedFormatter).toBeDefined();
73-
expect(undefinedFormatter).toBeInstanceOf(TimeFormatter);
74-
expect((undefinedFormatter as TimeFormatter).id).toBe(SMART_DATE_ID);
75-
76-
const emptyFormatter = getXAxisFormatter();
77-
expect(emptyFormatter).toBeDefined();
78-
expect(emptyFormatter).toBeInstanceOf(TimeFormatter);
79-
expect((emptyFormatter as TimeFormatter).id).toBe(SMART_DATE_ID);
80-
});
56+
test('getXAxisFormatter should return smart date formatter for undefined format', () => {
57+
const formatter = getXAxisFormatter();
58+
expect(formatter).toBeDefined();
59+
expect(formatter).toBeInstanceOf(TimeFormatter);
60+
expect((formatter as TimeFormatter).id).toBe(SMART_DATE_ID);
61+
});
8162

82-
test('time grain aware formatter should prevent millisecond and timestamp formats', () => {
83-
const formatter = getXAxisFormatter(SMART_DATE_ID, 'P1M');
84-
85-
// Test that dates with milliseconds don't show millisecond format
86-
const dateWithMs = new Date('2025-03-15T21:13:32.389Z');
87-
const result = (formatter as TimeFormatter).format(dateWithMs);
88-
expect(result).not.toContain('.389ms');
89-
expect(result).not.toMatch(/\.\d+ms/);
90-
expect(result).not.toContain('PM');
91-
expect(result).not.toContain('AM');
92-
expect(result).not.toMatch(/\d{1,2}:\d{2}/); // No time format
93-
});
63+
test('getXAxisFormatter should return custom time formatter for custom format', () => {
64+
const customFormat = '%Y-%m-%d';
65+
const formatter = getXAxisFormatter(customFormat);
66+
expect(formatter).toBeDefined();
67+
expect(formatter).toBeInstanceOf(TimeFormatter);
68+
expect((formatter as TimeFormatter).id).toBe(customFormat);
69+
});
9470

95-
test('time grain aware formatting should prevent problematic formats', () => {
96-
// Test that time grain aware formatter prevents the specific issues we solved
97-
const monthFormatter = getXAxisFormatter(SMART_DATE_ID, 'P1M');
98-
const yearFormatter = getXAxisFormatter(SMART_DATE_ID, 'P1Y');
99-
const dayFormatter = getXAxisFormatter(SMART_DATE_ID, 'P1D');
100-
101-
// Test dates that previously caused issues
102-
const problematicDates = [
103-
new Date('2025-03-15T21:13:32.389Z'), // Had .389ms issue
104-
new Date('2025-04-01T02:30:00.000Z'), // Timezone edge case
105-
new Date('2025-07-01T00:00:00.000Z'), // Month boundary
106-
];
107-
108-
problematicDates.forEach(date => {
109-
// Month formatter should not show milliseconds or PM/AM
110-
const monthResult = (monthFormatter as TimeFormatter).format(date);
111-
expect(monthResult).not.toMatch(/\.\d+ms/);
112-
expect(monthResult).not.toMatch(/PM|AM/);
113-
expect(monthResult).not.toMatch(/\d{1,2}:\d{2}:\d{2}/);
114-
115-
// Year formatter should not show milliseconds or PM/AM
116-
const yearResult = (yearFormatter as TimeFormatter).format(date);
117-
expect(yearResult).not.toMatch(/\.\d+ms/);
118-
expect(yearResult).not.toMatch(/PM|AM/);
119-
expect(yearResult).not.toMatch(/\d{1,2}:\d{2}:\d{2}/);
120-
121-
// Day formatter should not show milliseconds or seconds
122-
const dayResult = (dayFormatter as TimeFormatter).format(date);
123-
expect(dayResult).not.toMatch(/\.\d+ms/);
124-
expect(dayResult).not.toMatch(/:\d{2}:\d{2}/); // No seconds
125-
});
126-
});
71+
test('getXAxisFormatter smart date formatter should be returned and not undefined', () => {
72+
const formatter = getXAxisFormatter(SMART_DATE_ID);
73+
expect(formatter).toBeDefined();
74+
expect(formatter).toBeInstanceOf(TimeFormatter);
75+
expect((formatter as TimeFormatter).id).toBe(SMART_DATE_ID);
76+
77+
const undefinedFormatter = getXAxisFormatter(undefined);
78+
expect(undefinedFormatter).toBeDefined();
79+
expect(undefinedFormatter).toBeInstanceOf(TimeFormatter);
80+
expect((undefinedFormatter as TimeFormatter).id).toBe(SMART_DATE_ID);
81+
82+
const emptyFormatter = getXAxisFormatter();
83+
expect(emptyFormatter).toBeDefined();
84+
expect(emptyFormatter).toBeInstanceOf(TimeFormatter);
85+
expect((emptyFormatter as TimeFormatter).id).toBe(SMART_DATE_ID);
86+
});
12787

128-
test('time grain parameter should be passed correctly', () => {
129-
// Test that formatter with time grain is different from formatter without
130-
const formatterWithGrain = getXAxisFormatter(SMART_DATE_ID, 'P1M');
131-
const formatterWithoutGrain = getXAxisFormatter(SMART_DATE_ID);
132-
133-
expect(formatterWithGrain).toBeDefined();
134-
expect(formatterWithoutGrain).toBeDefined();
135-
expect(formatterWithGrain).toBeInstanceOf(TimeFormatter);
136-
expect(formatterWithoutGrain).toBeInstanceOf(TimeFormatter);
137-
138-
// Both should be valid formatters
139-
const testDate = new Date('2025-04-15T12:30:45.789Z');
140-
const resultWithGrain = (formatterWithGrain as TimeFormatter).format(
141-
testDate,
142-
);
143-
const resultWithoutGrain = (formatterWithoutGrain as TimeFormatter).format(
144-
testDate,
145-
);
146-
147-
expect(typeof resultWithGrain).toBe('string');
148-
expect(typeof resultWithoutGrain).toBe('string');
149-
expect(resultWithGrain.length).toBeGreaterThan(0);
150-
expect(resultWithoutGrain.length).toBeGreaterThan(0);
88+
test('getXAxisFormatter time grain aware formatter should prevent millisecond and timestamp formats', () => {
89+
const formatter = getXAxisFormatter(SMART_DATE_ID, TimeGranularity.MONTH);
90+
91+
// Test that dates with milliseconds don't show millisecond format
92+
const dateWithMs = new Date('2025-03-15T21:13:32.389Z');
93+
const result = (formatter as TimeFormatter).format(dateWithMs);
94+
expect(result).not.toContain('.389ms');
95+
expect(result).not.toMatch(/\.\d+ms/);
96+
expect(result).not.toContain('PM');
97+
expect(result).not.toContain('AM');
98+
expect(result).not.toMatch(/\d{1,2}:\d{2}/); // No time format
99+
});
100+
101+
test('getXAxisFormatter time grain aware formatting should prevent problematic formats', () => {
102+
// Test that time grain aware formatter prevents the specific issues we solved
103+
const monthFormatter = getXAxisFormatter(
104+
SMART_DATE_ID,
105+
TimeGranularity.MONTH,
106+
);
107+
const yearFormatter = getXAxisFormatter(SMART_DATE_ID, TimeGranularity.YEAR);
108+
const dayFormatter = getXAxisFormatter(SMART_DATE_ID, TimeGranularity.DAY);
109+
110+
// Test dates that previously caused issues
111+
const problematicDates = [
112+
new Date('2025-03-15T21:13:32.389Z'), // Had .389ms issue
113+
new Date('2025-04-01T02:30:00.000Z'), // Timezone edge case
114+
new Date('2025-07-01T00:00:00.000Z'), // Month boundary
115+
];
116+
117+
problematicDates.forEach(date => {
118+
// Month formatter should not show milliseconds or PM/AM
119+
const monthResult = (monthFormatter as TimeFormatter).format(date);
120+
expect(monthResult).not.toMatch(/\.\d+ms/);
121+
expect(monthResult).not.toMatch(/PM|AM/);
122+
expect(monthResult).not.toMatch(/\d{1,2}:\d{2}:\d{2}/);
123+
124+
// Year formatter should not show milliseconds or PM/AM
125+
const yearResult = (yearFormatter as TimeFormatter).format(date);
126+
expect(yearResult).not.toMatch(/\.\d+ms/);
127+
expect(yearResult).not.toMatch(/PM|AM/);
128+
expect(yearResult).not.toMatch(/\d{1,2}:\d{2}:\d{2}/);
129+
130+
// Day formatter should not show milliseconds or seconds
131+
const dayResult = (dayFormatter as TimeFormatter).format(date);
132+
expect(dayResult).not.toMatch(/\.\d+ms/);
133+
expect(dayResult).not.toMatch(/:\d{2}:\d{2}/); // No seconds
151134
});
135+
});
152136

153-
test('formatter without time grain should use standard smart date behavior', () => {
154-
const standardFormatter = getXAxisFormatter(SMART_DATE_ID);
155-
const timeGrainFormatter = getXAxisFormatter(SMART_DATE_ID, undefined);
137+
test('getXAxisFormatter time grain parameter should be passed correctly', () => {
138+
// Test that formatter with time grain is different from formatter without
139+
const formatterWithGrain = getXAxisFormatter(
140+
SMART_DATE_ID,
141+
TimeGranularity.MONTH,
142+
);
143+
const formatterWithoutGrain = getXAxisFormatter(SMART_DATE_ID);
144+
145+
expect(formatterWithGrain).toBeDefined();
146+
expect(formatterWithoutGrain).toBeDefined();
147+
expect(formatterWithGrain).toBeInstanceOf(TimeFormatter);
148+
expect(formatterWithoutGrain).toBeInstanceOf(TimeFormatter);
149+
150+
// Both should be valid formatters
151+
const testDate = new Date('2025-04-15T12:30:45.789Z');
152+
const resultWithGrain = (formatterWithGrain as TimeFormatter).format(
153+
testDate,
154+
);
155+
const resultWithoutGrain = (formatterWithoutGrain as TimeFormatter).format(
156+
testDate,
157+
);
158+
159+
expect(typeof resultWithGrain).toBe('string');
160+
expect(typeof resultWithoutGrain).toBe('string');
161+
expect(resultWithGrain.length).toBeGreaterThan(0);
162+
expect(resultWithoutGrain.length).toBeGreaterThan(0);
163+
});
156164

157-
// Both should be equivalent when no time grain is provided
158-
expect(standardFormatter).toBeDefined();
159-
expect(timeGrainFormatter).toBeDefined();
165+
test('getXAxisFormatter without time grain should use standard smart date behavior', () => {
166+
const standardFormatter = getXAxisFormatter(SMART_DATE_ID);
167+
const timeGrainFormatter = getXAxisFormatter(SMART_DATE_ID, undefined);
160168

161-
// Test with a date that has time components
162-
const testDate = new Date('2025-01-01T00:00:00.000Z');
163-
const standardResult = (standardFormatter as TimeFormatter).format(
164-
testDate,
165-
);
166-
const timeGrainResult = (timeGrainFormatter as TimeFormatter).format(
167-
testDate,
168-
);
169+
// Both should be equivalent when no time grain is provided
170+
expect(standardFormatter).toBeDefined();
171+
expect(timeGrainFormatter).toBeDefined();
169172

170-
expect(standardResult).toBe(timeGrainResult);
171-
});
173+
// Test with a date that has time components
174+
const testDate = new Date('2025-01-01T00:00:00.000Z');
175+
const standardResult = (standardFormatter as TimeFormatter).format(testDate);
176+
const timeGrainResult = (timeGrainFormatter as TimeFormatter).format(
177+
testDate,
178+
);
179+
180+
expect(standardResult).toBe(timeGrainResult);
172181
});

0 commit comments

Comments
 (0)