bito-code-review[bot] commented on code in PR #44664: URL: https://github.com/apache/superset/pull/44664#discussion_r4112736213
########## tests/unit_tests/utils/test_excel_conditional.py: ########## @@ -0,0 +1,153 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import io +from typing import Any + +from openpyxl import load_workbook, Workbook +from openpyxl.worksheet.worksheet import Worksheet + +from superset.utils.excel_conditional import apply_conditional_formatting + + +def _cf_rules(sheet: Worksheet) -> list[tuple[str, Any]]: + """Collect (range, rule) pairs from the first sheet's conditional formatting.""" + rules: list[tuple[str, Any]] = [] + for cf_range, cf_list in sheet.conditional_formatting._cf_rules.items(): # noqa: SLF001 + for rule in cf_list: + rules.append((str(cf_range), rule)) + return rules + + +def _workbook_with_sales(*values: float) -> bytes: + workbook = Workbook() + sheet = workbook.active + sheet["A1"] = "sales" + sheet["B1"] = "region" + for offset, value in enumerate(values, start=2): + sheet.cell(row=offset, column=1, value=value) + sheet["B2"] = "west" + raw = io.BytesIO() + workbook.save(raw) + return raw.getvalue() + + +def test_apply_conditional_formatting_writes_cell_is_rule() -> None: + """A greater-than rule becomes an Excel CellIs rule on the sales column.""" + styled = apply_conditional_formatting( + _workbook_with_sales(10, 50, 5), + [ + { + "column": "sales", + "operator": ">", + "targetValue": 8, + "colorScheme": "#FF0000", + } + ], + ) + result = load_workbook(io.BytesIO(styled)).active + rules = _cf_rules(result) + assert len(rules) == 1 + cf_range, rule = rules[0] + assert "A2" in cf_range + assert rule.type == "cellIs" + assert rule.operator == "greaterThan" + + +def test_apply_cell_bars_when_show_cell_bars_and_no_custom_rules() -> None: + """Table show_cell_bars adds data bars on numeric columns only.""" + styled = apply_conditional_formatting( + _workbook_with_sales(10), + [], + show_cell_bars=True, + ) + result = load_workbook(io.BytesIO(styled)).active + rules = _cf_rules(result) + assert any(rule.type == "dataBar" for _, rule in rules) + assert not any("B2" in cf_range for cf_range, _ in rules) + + +def test_cell_bar_object_writes_data_bar_only_on_matching_cells() -> None: + """CELL_BAR with a comparator covers matching cells, not the whole column.""" + styled = apply_conditional_formatting( + _workbook_with_sales(12.5, 5, 20), + [ + { + "column": "sales", + "operator": ">", + "targetValue": 10, + "colorScheme": "#5AC189", + "objectFormatting": "CELL_BAR", + } + ], + ) + result = load_workbook(io.BytesIO(styled)).active + rules = _cf_rules(result) + assert any(rule.type == "dataBar" for _, rule in rules) + cf_range = " ".join(rng for rng, rule in rules if rule.type == "dataBar") + assert "A2" in cf_range + assert "A4" in cf_range + assert "A3" not in cf_range Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Weak substring range assertions</b></div> <div id="fix"> These substring assertions cannot detect the regression this test exists for: a fallback to the full column range `A2:A4` (e.g. `_matching_cell_range` returning `_data_range`) passes all three checks, since "A3" is not a substring of "A2:A4", while "A2" also matches "A20:A25". Verified with an openpyxl 3.1.5 round-trip. Compare exact range tokens instead: `assert cf_range.split() == ["A2", "A4"]`. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion assert cf_range.split() == ["A2", "A4"] ```` </div> </details> </div> <small><i>Code Review Run #6fb400</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset/utils/excel_conditional.py: ########## @@ -0,0 +1,508 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Stamp Explore Table / Pivot Table v2 highlights onto an Excel workbook. + +Explore paints matching cells in the browser. Chart XLSX download is written +in ``QueryContextProcessor.get_data`` before client post-processing, so this +module is applied there as well as on the reports path. + +Rules from ``form_data["conditional_formatting"]`` become native Excel +conditional formatting (CellIs / formula / color scale / data bar). When that +list is empty and ``show_cell_bars`` is on, numeric columns get data bars so +the download matches the Table chart's default gradient. +""" + +from __future__ import annotations + +import io +from typing import Any, Mapping, Optional + +import pandas as pd +from openpyxl import load_workbook +from openpyxl.formatting.rule import ( + CellIsRule, + ColorScaleRule, + DataBarRule, + FormulaRule, +) +from openpyxl.styles import Font, PatternFill +from openpyxl.utils import get_column_letter +from openpyxl.workbook import Workbook +from openpyxl.worksheet.worksheet import Worksheet + +from superset.constants import SHOW_VALUES_AS_PERCENT_MODES +from superset.utils.excel_display import ( + apply_column_display, + refresh_sheet_bounds, + styles_from_pivot_form_data, + styles_from_table_form_data, +) + +# Theme tokens used by Table / Pivot Table v2 pickers, plus CSS names. +_NAMED_COLORS = { + "success": "52C41A", + "warning": "FAAD14", + "error": "FF4D4F", + "red": "FF4D4F", + "green": "52C41A", + "blue": "1890FF", + "yellow": "FAAD14", + "orange": "FA8C16", + "purple": "722ED1", + "cyan": "13C2C2", + "colorsuccess": "52C41A", + "colorwarning": "FAAD14", + "colorerror": "FF4D4F", + "colorsuccessbg": "F6FFED", + "colorwarningbg": "FFFBE6", + "colorerrorbg": "FFF2F0", +} + +_CELL_IS_OPERATORS = { + ">": "greaterThan", + "<": "lessThan", + ">=": "greaterThanOrEqual", + "<=": "lessThanOrEqual", + "=": "equal", + "==": "equal", + "!=": "notEqual", + "≠": "notEqual", + "≥": "greaterThanOrEqual", + "≤": "lessThanOrEqual", +} + +_RANGE_OPERATORS = { + "< x <": (">", "<"), + "< x ≤": (">", "<="), + "≤ x <": (">=", "<"), + "≤ x ≤": (">=", "<="), +} + +_DEFAULT_RULE_COLOR = _NAMED_COLORS["success"] +_DATA_BAR_POSITIVE = "63BE7B" +_SCALE_LOW = "FFFFFF" +_OBJECT_CELL_BAR = "CELL_BAR" +_OBJECT_TEXT = "TEXT_COLOR" +# First non-blank cells used to decide whether a column is numeric for +# default ``show_cell_bars``. Scanning the whole sheet is unnecessary for +# typical Table downloads; blank leading rows still skip until a value. +_NUMERIC_SAMPLE_ROWS = 20 + + +def _hex_rgb(color: Any) -> Optional[str]: + """Normalize a picker payload to a 6-digit RGB hex string.""" + if isinstance(color, dict): + hex_value = color.get("hex") + if isinstance(hex_value, str): + return _hex_rgb(hex_value) + red, green, blue = color.get("r"), color.get("g"), color.get("b") + if None not in (red, green, blue): + return f"{int(red):02X}{int(green):02X}{int(blue):02X}" + return None + if not isinstance(color, str) or not color: + return None + token = color.strip().lstrip("#") + named = _NAMED_COLORS.get(token.lower().replace("_", "").replace("-", "")) + if named: + return named + if len(token) == 3 and all(ch in "0123456789abcdefABCDEF" for ch in token): + return "".join(ch * 2 for ch in token).upper() + if len(token) >= 6 and all(ch in "0123456789abcdefABCDEF" for ch in token[:6]): + return token[:6].upper() + return None + + +def _rule_color(rule: dict[str, Any]) -> str: + return _hex_rgb(rule.get("colorScheme")) or _DEFAULT_RULE_COLOR + + +def _as_number(value: Any) -> Optional[float]: + if isinstance(value, bool) or value in (None, ""): + return None + if isinstance(value, (int, float)): + return float(value) + if isinstance(value, str): + try: + return float(value.replace(",", "")) + except ValueError: + return None + return None + + +def _value_matches_rule(value: Any, rule: dict[str, Any]) -> bool: + """Whether a cell should receive a CELL_BAR rule (Explore comparator).""" + if value in (None, ""): + return False + operator = rule.get("operator") + if operator in (None, "None", ""): + return True + number = _as_number(value) + if operator in _CELL_IS_OPERATORS: + target = rule.get("targetValue") + target_number = _as_number(target) + if number is not None and target_number is not None: + compare = _CELL_IS_OPERATORS[operator] + if compare == "greaterThan": + return number > target_number + if compare == "lessThan": + return number < target_number + if compare == "greaterThanOrEqual": + return number >= target_number + if compare == "lessThanOrEqual": + return number <= target_number + if compare == "equal": + return number == target_number + if compare == "notEqual": + return number != target_number + return str(value) == str(target) + if operator in _RANGE_OPERATORS: + left_op, right_op = _RANGE_OPERATORS[operator] + left = _as_number(rule.get("targetValueLeft")) + right = _as_number(rule.get("targetValueRight")) + if number is None or left is None or right is None: + return False + left_ok = number >= left if left_op == ">=" else number > left + right_ok = number <= right if right_op == "<=" else number < right + return left_ok and right_ok + return False + + +def _excel_literal(value: Any) -> str: + """Quote a comparison target so Excel treats it as a constant.""" + if isinstance(value, bool): + return "TRUE" if value else "FALSE" + if isinstance(value, (int, float)) and not isinstance(value, bool): + return str(value) + text = str(value).replace('"', '""') + return f'"{text}"' + + +def _header_matches(sheet_header: str, column: str) -> bool: + """Match a rule column to a header, including Pivot ``SUM(col)`` titles.""" + left = sheet_header.strip() + right = column.strip() + if left == right or left.lower() == right.lower(): + return True + return left.endswith(f"({right})") or left.endswith(f"({right.lower()})") + + +def _rule_column_aliases( + column: str, verbose_map: Optional[Mapping[str, Any]] +) -> list[str]: + """Raw and verbose names so rules still match after datasource rename.""" + aliases = [column] + if not verbose_map: + return aliases + verbose = verbose_map.get(column) + if verbose is not None and str(verbose) not in aliases: + aliases.append(str(verbose)) + for raw, label in verbose_map.items(): + if str(label) == column and str(raw) not in aliases: + aliases.append(str(raw)) + return aliases + + +def _column_index( + sheet: Worksheet, + header_row: int, + column: str, + verbose_map: Optional[Mapping[str, Any]] = None, +) -> Optional[int]: + aliases = _rule_column_aliases(column, verbose_map) + for col_idx in range(1, sheet.max_column + 1): + header_label = "" + for row in range(header_row, 0, -1): + value = sheet.cell(row=row, column=col_idx).value + if value not in (None, ""): + header_label = str(value) + break + if header_label and any(_header_matches(header_label, name) for name in aliases): + return col_idx + return None + + +def _data_range(sheet: Worksheet, col_idx: int, header_row: int) -> Optional[str]: + last_row = sheet.max_row + first_row = header_row + 1 + if last_row < first_row: + return None + letter = get_column_letter(col_idx) + return f"{letter}{first_row}:{letter}{last_row}" + + +def _solid_fill(rgb: str) -> PatternFill: + return PatternFill(start_color=rgb, end_color=rgb, fill_type="solid") + + +def _add_formula_rule( + sheet: Worksheet, + cell_range: str, + formula: str, + rgb: str, + *, + text_color: bool, +) -> None: + fill = None if text_color else _solid_fill(rgb) + font = Font(color=rgb) if text_color else None + sheet.conditional_formatting.add( + cell_range, + FormulaRule(formula=[formula], fill=fill, font=font), + ) + + +def _add_cell_is_rule( + sheet: Worksheet, + cell_range: str, + operator: str, + formula: list[str], + rgb: str, + *, + text_color: bool, +) -> None: + fill = None if text_color else _solid_fill(rgb) + font = Font(color=rgb) if text_color else None + sheet.conditional_formatting.add( + cell_range, + CellIsRule(operator=operator, formula=formula, fill=fill, font=font), + ) + + +def _add_data_bar(sheet: Worksheet, cell_range: str, rgb: str) -> None: + sheet.conditional_formatting.add( + cell_range, + DataBarRule( + start_type="min", + end_type="max", + color=rgb, + showValue=True, + minLength=None, + maxLength=None, + ), + ) + + +def _matching_cell_range( + sheet: Worksheet, + col_idx: int, + header_row: int, + rule: dict[str, Any], +) -> Optional[str]: + """Union of cells that match a CELL_BAR comparator, or the full column.""" + operator = rule.get("operator") + if operator in (None, "None", ""): + return _data_range(sheet, col_idx, header_row) + letter = get_column_letter(col_idx) + first_row = header_row + 1 + last_row = sheet.max_row + if last_row < first_row: + return None + runs: list[tuple[int, int]] = [] + run_start: Optional[int] = None + for row in range(first_row, last_row + 1): + matched = _value_matches_rule(sheet.cell(row=row, column=col_idx).value, rule) + if matched and run_start is None: + run_start = row + elif not matched and run_start is not None: + runs.append((run_start, row - 1)) + run_start = None + if run_start is not None: + runs.append((run_start, last_row)) + if not runs: + return None + parts = [ + f"{letter}{start}" if start == end else f"{letter}{start}:{letter}{end}" + for start, end in runs + ] + return " ".join(parts) + + +def _apply_rule( + sheet: Worksheet, + header_row: int, + rule: dict[str, Any], + verbose_map: Optional[Mapping[str, Any]] = None, +) -> None: + column = rule.get("column") + if not isinstance(column, str) or not column: + return + col_idx = _column_index(sheet, header_row, column, verbose_map) + if col_idx is None: + return + cell_range = _data_range(sheet, col_idx, header_row) + if cell_range is None: + return + + rgb = _rule_color(rule) + operator = rule.get("operator") + object_fmt = rule.get("objectFormatting") or "" + text_color = object_fmt == _OBJECT_TEXT + top_left = cell_range.split(":", 1)[0] + + if object_fmt == _OBJECT_CELL_BAR: + bar_range = _matching_cell_range(sheet, col_idx, header_row, rule) + if bar_range: + _add_data_bar(sheet, bar_range, rgb) + return + + if operator in _CELL_IS_OPERATORS: + _add_cell_is_rule( + sheet, + cell_range, + _CELL_IS_OPERATORS[operator], + [_excel_literal(rule.get("targetValue"))], + rgb, + text_color=text_color, + ) + return + + if operator in _RANGE_OPERATORS: + left_op, right_op = _RANGE_OPERATORS[operator] + left = _excel_literal(rule.get("targetValueLeft")) + right = _excel_literal(rule.get("targetValueRight")) + _add_formula_rule( + sheet, + cell_range, + ( + f"AND(NOT(ISBLANK({top_left}))," + f"{top_left}{left_op}{left}," + f"{top_left}{right_op}{right})" + ), + rgb, + text_color=text_color, + ) + return + + if operator in (None, "None", ""): + if text_color: + _add_formula_rule( + sheet, + cell_range, + f"NOT(ISBLANK({top_left}))", + rgb, + text_color=True, + ) + return + min_bound = _as_number(rule.get("minBound")) + max_bound = _as_number(rule.get("maxBound")) + start_type = "num" if min_bound is not None else "min" + end_type = "num" if max_bound is not None else "max" + sheet.conditional_formatting.add( + cell_range, + ColorScaleRule( + start_type=start_type, + start_value=min_bound, + start_color=_SCALE_LOW, + end_type=end_type, + end_value=max_bound, Review Comment: <div> <div id="suggestion"> <div id="issue"><b>ColorScale num with None val</b></div> <div id="fix"> `start_value=min_bound` / `end_value=max_bound` are passed even when the bound is None and the type falls back to "min"/"max". openpyxl 3.1.5's `ColorScaleRule` wraps every non-None type in a `FormatObject(type=..., val=...)` with no validation, so a `type="num"` cfvo with `val=None` can be emitted (e.g. `minBound` absent but `maxBound="abc"`), producing a color scale Excel may reject or misread. Pass the value only when the type is "num". </div> </div> <small><i>Code Review Run #6fb400</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
