michael-s-molina commented on code in PR #42088:
URL: https://github.com/apache/superset/pull/42088#discussion_r3646160312
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts:
##########
@@ -55,29 +55,63 @@ const getCellStyle = (params: CellStyleParams) => {
let backgroundColor;
let color;
if (hasColumnColorFormatters) {
+ const applyFormatter = (
+ formatter: ColorFormatters[number],
+ valueToFormat: typeof value,
+ ) => {
+ const formatterResult =
+ valueToFormat || valueToFormat === 0
+ ? formatter.getColorFromValue(valueToFormat)
+ : false;
+ if (formatterResult) {
+ if (
+ formatter.objectFormatting === ObjectFormattingEnum.TEXT_COLOR ||
+ formatter.toTextColor
+ ) {
+ color = formatterResult;
+ } else if (
+ formatter.objectFormatting !== ObjectFormattingEnum.CELL_BAR
+ ) {
+ backgroundColor = formatterResult;
+ }
+ }
+ };
+
+ // formatter.column can be a legacy display label ("Main colname") for
+ // time-comparison columns rather than the row's actual data key, so
+ // resolve it to the real field id before using it to read row values.
+ const resolveColumnKey = (columnKey: string) =>
+ columnKey.includes('Main')
+ ? columnKey.replace('Main', '').trim()
+ : columnKey;
Review Comment:
Confirmed and fixed in 2c3c0e74ac: `resolveColumnKey` now only strips the
exact `'Main '` prefix (matching the pattern already used in
`controlPanel.tsx`) instead of replacing the substring anywhere in the string,
so legitimate column names containing "Main" are no longer mutated.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts:
##########
@@ -55,29 +55,63 @@ const getCellStyle = (params: CellStyleParams) => {
let backgroundColor;
let color;
if (hasColumnColorFormatters) {
+ const applyFormatter = (
+ formatter: ColorFormatters[number],
+ valueToFormat: typeof value,
+ ) => {
+ const formatterResult =
+ valueToFormat || valueToFormat === 0
+ ? formatter.getColorFromValue(valueToFormat)
+ : false;
+ if (formatterResult) {
+ if (
+ formatter.objectFormatting === ObjectFormattingEnum.TEXT_COLOR ||
+ formatter.toTextColor
+ ) {
+ color = formatterResult;
+ } else if (
+ formatter.objectFormatting !== ObjectFormattingEnum.CELL_BAR
+ ) {
+ backgroundColor = formatterResult;
+ }
+ }
+ };
+
+ // formatter.column can be a legacy display label ("Main colname") for
+ // time-comparison columns rather than the row's actual data key, so
+ // resolve it to the real field id before using it to read row values.
+ const resolveColumnKey = (columnKey: string) =>
+ columnKey.includes('Main')
+ ? columnKey.replace('Main', '').trim()
+ : columnKey;
+
columnColorFormatters!
.filter(formatter => {
- const colTitle = formatter?.column?.includes('Main')
- ? formatter?.column?.replace('Main', '').trim()
- : formatter?.column;
- return colTitle === colDef.field;
+ if (formatter.columnFormatting) {
+ return formatter.columnFormatting === colDef.field;
+ }
+ return resolveColumnKey(formatter.column) === colDef.field;
Review Comment:
Fixed in 2c3c0e74ac. Note: `columnFormatting` is currently only ever set to
`undefined` or `ObjectFormattingEnum.ENTIRE_ROW` (see `controlPanel.tsx`), so
this wasn't a live bug today, but the comparison was misleading. Rewrote the
filter to explicitly exclude entire-row formatters from the per-column loop
instead of relying on an incidental enum/field-name mismatch.
##########
superset/migrations/shared/migrate_viz/base.py:
##########
@@ -185,20 +199,35 @@ def upgrade_slice(cls, slc: Slice) -> None:
def downgrade_slice(cls, slc: Slice) -> None:
try:
form_data = try_load_json(slc.params)
- if "viz_type" in (
+ if "viz_type" not in (
form_data_bak := form_data.get(FORM_DATA_BAK_FIELD_NAME, {})
):
- slc.params = json.dumps(form_data_bak)
- slc.viz_type = form_data_bak.get("viz_type")
- query_context = try_load_json(slc.query_context)
- queries_bak = form_data.get(QUERIES_BAK_FIELD_NAME, {})
- if queries_bak:
- query_context["queries"] = queries_bak
- if "form_data" in query_context:
- query_context["form_data"] = form_data_bak
- slc.query_context = json.dumps(query_context)
- else:
- slc.query_context = None
+ return
+
+ new_params = json.dumps(form_data_bak)
+ new_viz_type = form_data_bak.get("viz_type")
+
+ # Sentinel so a "leave query_context untouched" branch below is
+ # distinguishable from "explicitly set it to None".
+ unchanged = object()
+ new_query_context: Any = unchanged
+
+ query_context = try_load_json(slc.query_context)
+ queries_bak = form_data.get(QUERIES_BAK_FIELD_NAME, {})
+ if queries_bak:
+ query_context["queries"] = queries_bak
+ if "form_data" in query_context:
+ query_context["form_data"] = form_data_bak
+ new_query_context = json.dumps(query_context)
Review Comment:
Confirmed and fixed in 2c3c0e74ac: `new_query_context` is now serialized
whenever `queries_bak` is restored, regardless of whether `form_data` is
present in the context, so restored queries are no longer silently dropped on
downgrade.
--
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]
