diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts b/superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts index 10b5c996d6f..dbff332cdc0 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts @@ -350,7 +350,7 @@ export const getReadableTextColor = ( const { r: bgR, g: bgG, b: bgB, a: bgAlpha } = background.toRgb(); const { r: surfaceR, g: surfaceG, b: surfaceB } = surface.toRgb(); - const alpha = bgAlpha ?? 1; + const alpha = bgAlpha; const compositeColor = tinycolor({ r: bgR * alpha + surfaceR * (1 - alpha), diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/utils/getColorFormatters.test.ts b/superset-frontend/packages/superset-ui-chart-controls/test/utils/getColorFormatters.test.ts index 7b437f68f63..821a864a546 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/test/utils/getColorFormatters.test.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/test/utils/getColorFormatters.test.ts @@ -129,6 +129,11 @@ test('getReadableTextColor blends alpha over the provided surface', () => { ); }); +test('getReadableTextColor returns undefined for invalid colors', () => { + expect(getReadableTextColor('not-a-color', '#ffffff')).toBeUndefined(); + expect(getReadableTextColor('#111111', 'not-a-color')).toBeUndefined(); +}); + test('getTextColorForBackground prefers explicit text color', () => { expect( getTextColorForBackground( @@ -145,6 +150,10 @@ test('getNormalizedTextColor removes alpha from explicit text colors', () => { ); }); +test('getNormalizedTextColor preserves invalid explicit text colors', () => { + expect(getNormalizedTextColor('not-a-color')).toBe('not-a-color'); +}); + test('getTextColorForBackground normalizes explicit text color alpha', () => { expect( getTextColorForBackground( diff --git a/superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridGenerator.test.ts b/superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridGenerator.test.ts index a781202eb8c..047105ed56b 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridGenerator.test.ts +++ b/superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridGenerator.test.ts @@ -196,6 +196,25 @@ test('should handle string metrics', () => { expect(grid!.colHeaders).toEqual(['avg', 'max']); }); +test('should skip missing column metrics when generating cell form data', () => { + const missingColumnMetricFormData: TestFormData = { + viz_type: 'table', + datasource: '1__table', + matrixify_enable: true, + matrixify_mode_rows: 'metrics', + matrixify_mode_columns: 'metrics', + matrixify_rows: [createAdhocMetric('Revenue')], + matrixify_columns: [null], + }; + + const grid = generateMatrixifyGrid(missingColumnMetricFormData); + + expect(grid).not.toBeNull(); + expect(grid!.cells[0][0]!.formData.metrics).toEqual([ + createAdhocMetric('Revenue'), + ]); +}); + test('should not escape HTML entities in cell titles', () => { const formDataWithSpecialChars: TestFormData = { viz_type: 'table', diff --git a/superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.test.ts b/superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.test.ts index f23b252af00..81c36a0cca2 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.test.ts +++ b/superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.test.ts @@ -149,6 +149,15 @@ test('getMatrixifyConfig should return null when matrixify_enable is false', () expect(getMatrixifyConfig(formData)).toBeNull(); }); +test('getMatrixifyConfig should return null when no axes are enabled', () => { + const formData = { + viz_type: 'table', + matrixify_enable: true, + } as MatrixifyFormData; + + expect(getMatrixifyConfig(formData)).toBeNull(); +}); + test('getMatrixifyConfig should return valid config for metrics mode', () => { const formData = { viz_type: 'table', @@ -212,6 +221,24 @@ test('getMatrixifyConfig should handle topn selection mode', () => { expect(config!.rows.dimension).toEqual(formData.matrixify_dimension_rows); }); +test('getMatrixifyConfig should preserve ascending topn order when explicitly disabled', () => { + const formData = { + viz_type: 'table', + matrixify_enable: true, + matrixify_mode_rows: 'dimensions', + matrixify_mode_columns: 'dimensions', + matrixify_dimension_rows: { dimension: 'country', values: ['USA'] }, + matrixify_dimension_columns: { dimension: 'product', values: ['Widget'] }, + matrixify_topn_order_rows: false, + matrixify_topn_order_columns: false, + } as MatrixifyFormData; + + const config = getMatrixifyConfig(formData); + expect(config).not.toBeNull(); + expect(config!.rows.topnOrder).toBe('asc'); + expect(config!.columns.topnOrder).toBe('asc'); +}); + test('getMatrixifyValidationErrors should return empty array when matrixify is not enabled', () => { const formData = { viz_type: 'table', @@ -256,6 +283,15 @@ test('getMatrixifyValidationErrors should return empty array when properly confi expect(getMatrixifyValidationErrors(formData)).toEqual([]); }); +test('getMatrixifyValidationErrors should return empty array when enabled with no active axes', () => { + const formData = { + viz_type: 'table', + matrixify_enable: true, + } as MatrixifyFormData; + + expect(getMatrixifyValidationErrors(formData)).toEqual([]); +}); + test('getMatrixifyValidationErrors should return error when enabled but no configuration exists', () => { const formData = { viz_type: 'table', diff --git a/superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.ts b/superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.ts index 4c61c810d6c..b179daf1fbf 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.ts +++ b/superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.ts @@ -200,10 +200,7 @@ export function isMatrixifyEnabled(formData: MatrixifyFormData): boolean { return false; } - const config = getMatrixifyConfig(formData); - if (!config) { - return false; - } + const config = getMatrixifyConfig(formData)!; const hasRowData = rowEnabled && @@ -247,11 +244,7 @@ export function getMatrixifyValidationErrors( return errors; } - const config = getMatrixifyConfig(formData); - if (!config) { - errors.push('Please configure at least one row or column axis'); - return errors; - } + const config = getMatrixifyConfig(formData)!; // Check row configuration (only if explicitly set in form data) const hasRowMode = Boolean(formData.matrixify_mode_rows); diff --git a/superset/datasets/schemas.py b/superset/datasets/schemas.py index 1a30a6bd799..386686d5700 100644 --- a/superset/datasets/schemas.py +++ b/superset/datasets/schemas.py @@ -167,6 +167,7 @@ class DatasetPostSchema(Schema): external_url = fields.String(allow_none=True) normalize_columns = fields.Boolean(load_default=False) always_filter_main_dttm = fields.Boolean(load_default=False) + currency_code_column = fields.String(allow_none=True, validate=Length(0, 250)) template_params = fields.String(allow_none=True) uuid = fields.UUID(allow_none=True) diff --git a/tests/integration_tests/datasets/api_tests.py b/tests/integration_tests/datasets/api_tests.py index c0692fe9741..d250b34958f 100644 --- a/tests/integration_tests/datasets/api_tests.py +++ b/tests/integration_tests/datasets/api_tests.py @@ -861,6 +861,29 @@ class TestDatasetApi(SupersetTestCase): assert alpha in model.owners self.items_to_delete = [model] + @pytest.mark.usefixtures("load_energy_table_with_slice") + def test_create_dataset_with_currency_code_column(self): + """ + Dataset API: Test create dataset persists currency_code_column + """ + + energy_usage_ds = self.get_energy_usage_dataset() + self.login(ALPHA_USERNAME) + table_data = { + "database": energy_usage_ds.database_id, + "table_name": "energy_usage_virtual_currency_column", + "sql": "select * from energy_usage", + "currency_code_column": "currency", + } + if schema := get_example_default_schema(): + table_data["schema"] = schema + rv = self.post_assert_metric("/api/v1/dataset/", table_data, "post") + assert rv.status_code == 201 + data = json.loads(rv.data.decode("utf-8")) + model = db.session.query(SqlaTable).get(data.get("id")) + assert model.currency_code_column == "currency" + self.items_to_delete = [model] + @unittest.skip("test is failing stochastically") def test_create_dataset_same_name_different_schema(self): if backend() == "sqlite": diff --git a/tests/unit_tests/datasets/schema_tests.py b/tests/unit_tests/datasets/schema_tests.py index 40d63f3af8b..dcea6e795a6 100644 --- a/tests/unit_tests/datasets/schema_tests.py +++ b/tests/unit_tests/datasets/schema_tests.py @@ -48,35 +48,60 @@ def test_validate_python_date_format_raises(payload) -> None: validate_python_date_format(payload) -def test_dataset_put_schema_includes_currency_code_column() -> None: - """Test that DatasetPutSchema properly handles currency_code_column field.""" - from superset.datasets.schemas import DatasetPutSchema +def test_dataset_post_schema_has_all_put_scalar_fields() -> None: + """ + Every scalar model field accepted by DatasetPutSchema should also be accepted + by DatasetPostSchema, unless it is intentionally excluded (fields that only + make sense after a dataset already exists). - schema = DatasetPutSchema() + This prevents the class of bug where a new column is added to the update + schema but forgotten in the create schema. + """ + from superset.datasets.schemas import DatasetPostSchema, DatasetPutSchema - # Dataset with currency code column + # Fields that are intentionally only on Put: they require an existing dataset + # or are populated server-side during creation. + put_only_fields = { + "columns", + "metrics", + "folders", + "database_id", # Post uses "database" (integer id) instead + "description", + "main_dttm_col", + "filter_select_enabled", + "fetch_values_predicate", + "offset", + "default_endpoint", + "cache_timeout", + "is_sqllab_view", + "extra", + } + + put_fields = set(DatasetPutSchema().fields.keys()) + post_fields = set(DatasetPostSchema().fields.keys()) + + missing = put_fields - post_fields - put_only_fields + assert missing == set(), ( + f"Fields {missing} are in DatasetPutSchema but missing from " + f"DatasetPostSchema. Either add them to DatasetPostSchema or to " + f"the put_only_fields exclusion list in this test." + ) + + +def test_dataset_post_schema_includes_currency_code_column() -> None: + """Test that DatasetPostSchema accepts currency_code_column.""" + from superset.datasets.schemas import DatasetPostSchema + + schema = DatasetPostSchema() data = { + "database": 1, + "table_name": "virtual_dataset", "currency_code_column": "currency", } result = schema.load(data) assert result["currency_code_column"] == "currency" -def test_dataset_put_schema_currency_code_column_optional() -> None: - """Test that currency_code_column is optional in DatasetPutSchema.""" - from superset.datasets.schemas import DatasetPutSchema - - schema = DatasetPutSchema() - - # Dataset without currency code column (should not fail) - data: dict[str, str | None] = {} - result = schema.load(data) - assert ( - "currency_code_column" not in result - or result.get("currency_code_column") is None - ) - - def test_dataset_metrics_put_schema_parses_currency_string() -> None: """Test that DatasetMetricsPutSchema parses string currency payloads.""" from superset.datasets.schemas import DatasetMetricsPutSchema