jesperct commented on code in PR #40865:
URL: https://github.com/apache/superset/pull/40865#discussion_r3388812507
##########
superset-frontend/src/explore/controlUtils/standardizedFormData.test.ts:
##########
@@ -344,6 +344,47 @@ describe('should collect control values and create SFD',
() => {
]);
});
+ test('strips inherit/custom time shifts when target viz does not support
them', () => {
+ // Table/Big Number period-over-period offer "inherit" and "custom" time
+ // shifts; the timeseries advanced-analytics target (target_viz) does not.
+ // Carrying them over leaves un-removable tags on the new chart (SC-99170).
+ const store = {
+ ...sourceMockStore,
+ form_data: {
+ ...sourceMockFormData,
+ time_compare: ['1 year ago', 'inherit', 'custom'],
+ },
+ };
+ const sfd = new StandardizedFormData(store.form_data);
+ const { formData } = sfd.transform('target_viz', store);
+ // the free-form delta survives, the special markers are dropped
+ expect(formData.time_compare).toEqual(['1 year ago']);
+ });
+
+ test('preserves inherit/custom time shifts when target viz supports them',
() => {
+ getChartControlPanelRegistry().registerValue('target_viz_inherit', {
+ controlPanelSections: [
+ sections.timeComparisonControls({}),
+ {
+ label: 'transform controls',
+ controlSetRows: publicControls
+ .filter(c => c !== 'dashboardId' && c !== 'time_compare')
+ .map(control => [control]),
+ },
+ ],
+ });
+ const store = {
+ ...sourceMockStore,
+ form_data: {
+ ...sourceMockFormData,
+ time_compare: ['1 year ago', 'inherit'],
+ },
+ };
+ const sfd = new StandardizedFormData(store.form_data);
+ const { formData } = sfd.transform('target_viz_inherit', store);
+ expect(formData.time_compare).toEqual(['1 year ago', 'inherit']);
+ });
Review Comment:
Good call. Added a test that converts a chart with `time_compare:
['inherit', 'custom']` to a viz without those choices and asserts the value
comes out `null`, so the empty-result branch is locked in.
##########
superset-frontend/src/explore/controlUtils/standardizedFormData.ts:
##########
@@ -156,6 +156,36 @@ export class StandardizedFormData {
return controls;
}
+ // Time shifts only meaningful for viz types whose "Time shift" control
offers
+ // them (Big Number / Table period-over-period). Other viz types reuse the
same
+ // `time_compare` key without these choices.
+ private static specialTimeShifts = ['inherit', 'custom'];
+
+ // Drop `time_compare` markers the target viz can't honor so they don't carry
+ // over as un-removable tags when switching chart types.
+ static dropUnsupportedTimeShifts(
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ controlsState: Record<string, any>,
+ ): void {
+ const control = controlsState?.time_compare;
+ if (!control || !Array.isArray(control.value)) {
+ return;
+ }
+ const supportedChoices = new Set(
+ ensureIsArray(control.choices).map(
+ (choice: [string, string]) => choice[0],
+ ),
+ );
+ const filtered = control.value.filter(
+ (shift: string) =>
+ !StandardizedFormData.specialTimeShifts.includes(shift) ||
+ supportedChoices.has(shift),
+ );
+ if (filtered.length !== control.value.length) {
+ control.value = filtered.length ? filtered : null;
+ }
+ }
Review Comment:
Switched the parameter to `Record<string, unknown>` and added runtime guards
on the `value` and `choices` shapes, so it no longer leans on `any` or assumes
the choices tuple type.
--
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]
