From 6f211556a58270e777280a1f48a97cdc87fe6f71 Mon Sep 17 00:00:00 2001 From: Jan Tychtl Date: Fri, 13 Nov 2026 12:29:44 +0100 Subject: [PATCH] feat(gooddata-eval): score widgets inside dashboard containers The dashboard evaluator read only a tab's top-level widgets. A chart moved into a container counted as missing, and an item could not state a container's children, their order or its direction, so creating a group, stacking it or swapping inside it had nothing to be scored by. Every chart check now reads the leaf widgets inside containers; a switcher stays one widget. A widget is a container when it carries `container` or `sections`, because the AAC converter writes `sections` only for a non-empty container. A `layout` entry can name a container by `container_path` and state its `layout_direction`, and a section can list its exact children with `widgets`: chart ids, authored chart titles, notes and whole switchers. Entries are keyed by tab and path, so one tab can carry several. The same tab and path twice, and an entry without tab beside a named one, are still refused; `filter_bar` keeps its one-entry-per-tab rule. Documents without containers and existing layout items score as before. jira: GDAI-2416 risk: low --- .../core/agentic/dashboard_skill.py | 248 ++++++++++- .../tests/test_agentic_dashboard_skill.py | 397 ++++++++++++++++++ 2 files changed, 622 insertions(+), 23 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/dashboard_skill.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/dashboard_skill.py index 6fe2a304c..47bfc2dff 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/dashboard_skill.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/dashboard_skill.py @@ -186,16 +186,41 @@ def _sections_of(dashboard: dict) -> list[dict]: def _widgets_of(dashboard: dict) -> list[dict]: - """Every widget of the dashboard, flattened across sections. + """Every leaf widget of the dashboard, flattened across sections and the containers in them. Charts are matched dashboard-wide on purpose: the expectation names charts, not a layout, - so which section or tab one lands on is the agent's call. The date filter is deliberately - the opposite -- ``_check_date_range`` requires *every* tab to match. That one is defensive - rather than observed: ``draft_dashboard`` copies a single filter onto every tab + so which section, tab or container one lands on is the agent's call. The date filter is + deliberately the opposite -- ``_check_date_range`` requires *every* tab to match. That one is + defensive rather than observed: ``draft_dashboard`` copies a single filter onto every tab unconditionally, so a draft cannot currently disagree with itself across tabs, and the per-tab check is there to notice if that stops being true. """ - return [widget for section in _sections_of(dashboard) for widget in section.get("widgets") or []] + return _leaf_widgets(_sections_of(dashboard)) + + +def _is_container(widget: dict) -> bool: + """Whether a widget is a container, named by either of its two keys. + + The AAC converter writes ``container`` on every container but ``sections`` only when it has + some, so a saved empty container arrives with ``container`` alone. + """ + return "container" in widget or "sections" in widget + + +def _leaf_widgets(sections: list[dict]) -> list[dict]: + """The widgets of ``sections`` in document order, each container replaced by the widgets inside it. + + A switcher stays one widget, as it does at the top level, so the chart checks reading this + list do not see its member charts. + """ + leaves: list[dict] = [] + for section in sections: + for widget in section.get("widgets") or []: + if _is_container(widget): + leaves.extend(_leaf_widgets(widget.get("sections") or [])) + else: + leaves.append(widget) + return leaves def _apply_patch(base: dict, operations: list[dict]) -> tuple[dict | None, str | None]: @@ -747,7 +772,7 @@ def _check_tabs(dashboard: dict, new_ids: set[str], expected: list[dict]) -> lis for index, (tab, exp) in enumerate(zip(tabs, expected)): if "title" in exp and _norm(str(tab.get("title") or "")) != _norm(str(exp["title"] or "")): failures.append(f"tab {index} is titled {tab.get('title')!r}, expected {exp['title']!r}") - tab_widgets = [w for section in tab.get("sections") or [] for w in section.get("widgets") or []] + tab_widgets = _leaf_widgets(tab.get("sections") or []) failures.extend( f"tab {index} ({tab.get('title')!r}) does not hold chart {chart!r}" for chart in exp.get("visualizations") or [] @@ -902,15 +927,20 @@ def _named_tab(dashboard: dict, tab_id: str | None) -> tuple[dict | None, str]: def _check_layout(dashboard: dict, new_ids: set[str], expected: list[dict]) -> list[str]: - """Each named tab must hold exactly the expected sections, in order, each with exactly its charts in order. + """Each named layout must hold exactly the expected sections, in order, each with exactly its widgets in order. Exact on purpose. A move that leaves the widget where it was, places a second copy, or puts it in the wrong section keeps every chart "on the dashboard", and so does a removal that - never happened; only the whole arrangement tells them apart. A section's ``title`` is - compared when the entry states it, ``null`` meaning untitled. Its ``visualizations`` are - always stated, ``[]`` for a section without charts. A chart is an id, or - ``{"title": ...}`` for one the agent authored. Widgets that show no visualization are not - charts and are left out on both sides. + never happened; only the whole arrangement tells them apart. + + The layout is a tab's own, or with ``container_path`` the container that path reaches from + it, compared on ``layout_direction`` too when the entry states it. A section's ``title`` is + compared when the entry states it, ``null`` meaning untitled. Its ``visualizations`` list the + charts, ``[]`` for a section without any, and the widgets that show no visualization are + left out on both sides. Its ``widgets`` instead list every child: a chart is an id, or + ``{"title": ...}`` for one the agent authored, a note ``{"text": ...}`` and a switcher + ``{"switcher": [charts]}``, which is how a swap of a note and a chart, or a switcher taken + apart, is told from the arrangement asked for. """ failures: list[str] = [] for entry in expected: @@ -919,6 +949,16 @@ def _check_layout(dashboard: dict, new_ids: set[str], expected: list[dict]) -> l failures.append(where) continue sections = tab.get("sections") or [] + if "container_path" in entry: + container, where = _container_at(sections, entry["container_path"], where) + if container is None: + failures.append(where) + continue + sections = container.get("sections") or [] + # The converter writes no direction for a row, and the dashboard lays a container out as one. + direction = container.get("layout_direction") or "row" + if "layout_direction" in entry and direction != entry["layout_direction"]: + failures.append(f"{where} is laid out as a {direction}, expected a {entry['layout_direction']}") wanted = entry["sections"] if len(sections) != len(wanted): titles = ", ".join(repr(s.get("title")) for s in sections) @@ -929,6 +969,14 @@ def _check_layout(dashboard: dict, new_ids: set[str], expected: list[dict]) -> l failures.append( f"{where} section {index} is titled {section.get('title')!r}, expected {want['title']!r}" ) + if "widgets" in want: + children = section.get("widgets") or [] + if len(children) != len(want["widgets"]) or not all( + _holds(w, new_ids, c) for w, c in zip(children, want["widgets"], strict=True) + ): + shown = [_widget_entry(w) for w in children] + failures.append(f"{where} section {index} holds {shown}, expected {want['widgets']}") + continue charts = [w for w in section.get("widgets") or [] if w.get("visualization")] charts_wanted = want["visualizations"] if len(charts) != len(charts_wanted) or not all( @@ -939,6 +987,64 @@ def _check_layout(dashboard: dict, new_ids: set[str], expected: list[dict]) -> l return failures +def _container_at(sections: list[dict], path: list[dict], where: str) -> tuple[dict | None, str]: + """The container ``path`` reaches from ``sections``, and how to name it in a failure. + + Each hop is a section and a widget index inside the layout the previous hop reached. A hop + that lands on nothing, or on a widget that is not a container, gives ``None`` and the reason: + the result left no container where the case expects one, which fails the case. + """ + container: dict | None = None + for depth, hop in enumerate(path): + at = f"section {hop['section']}, widget_index {hop['widget_index']}" + widgets = (sections[hop["section"]].get("widgets") or []) if hop["section"] < len(sections) else [] + if hop["widget_index"] >= len(widgets): + return None, f"{where} has no widget at {at}" + container = widgets[hop["widget_index"]] + if not _is_container(container): + return None, f"{where} holds {_widget_entry(container)!r} at {at}, not a container" + where = f"{where} {'container at' if depth == 0 else '>'} {at}" + sections = container.get("sections") or [] + return container, where + + +def _holds(widget: dict, new_ids: set[str], expected: str | dict) -> bool: + """Whether ``widget`` is the child a layout section's ``widgets`` entry names. + + The widget is read as the kind ``_widget_entry`` names it, so a widget carrying the keys of + two kinds matches only the one it renders as. + """ + shown = _widget_entry(widget) + if isinstance(expected, dict) and "text" in expected: + return shown == expected + if isinstance(expected, dict) and "switcher" in expected: + members = widget.get("visualizations") or [] + return ( + isinstance(shown, dict) + and "switcher" in shown + and len(members) == len(expected["switcher"]) + and all( + _matching_widgets([m], new_ids, _chart_entry(c)) + for m, c in zip(members, expected["switcher"], strict=True) + ) + ) + return not isinstance(shown, dict) and bool(_matching_widgets([widget], new_ids, _chart_entry(expected))) + + +def _widget_entry(widget: dict) -> object: + """A child widget written the way a layout section's ``widgets`` entry names it, for a failure message. + + Kinds are read in the converter's order, container first, so a widget is named as it renders. + """ + if _is_container(widget): + return {"container": widget.get("container")} + if "visualizations" in widget: + return {"switcher": [m.get("visualization") for m in widget["visualizations"] or []]} + if "content" in widget: + return {"text": widget["content"]} + return widget.get("visualization") + + def _check_filter_bar(dashboard: dict, expected: list[dict]) -> list[str]: """Each named tab must carry exactly the expected filters: the kept ones by key, the new ones by content. @@ -1416,24 +1522,34 @@ def _validate_layout_expectations(expected_output: dict) -> None: raise ValueError(f"a preserved widget needs its visualization and a key to guard, got {widget!r}") if "layout" in expected_output: - for entry in _tab_entries(expected_output, "layout", {"tab", "sections"}): + for entry in _layout_entries(expected_output): sections = entry.get("sections") if not isinstance(sections, list): raise ValueError(f"a layout entry needs its sections as a list, got {entry!r}") for section in sections: - if not isinstance(section, dict) or section.keys() - {"title", "visualizations"}: - raise ValueError(f"a layout section takes only title and visualizations, got {section!r}") + if not isinstance(section, dict) or section.keys() - {"title", "visualizations", "widgets"}: + raise ValueError( + f"a layout section takes only title and visualizations or widgets, got {section!r}" + ) if section.get("title") is not None and not isinstance(section["title"], str): raise ValueError(f"a layout section's title must be a string or null, got {section!r}") - if "visualizations" not in section: + if ("visualizations" in section) == ("widgets" in section): # Missing would read as "no charts" here but as "any charts" in `tabs`. - raise ValueError(f"a layout section must list its visualizations, [] for none, got {section!r}") + raise ValueError( + f"a layout section must list either its visualizations or its widgets, [] for none, " + f"got {section!r}" + ) + if "widgets" in section: + if not isinstance(section["widgets"], list) or not all( + _is_widget_entry(w) for w in section["widgets"] + ): + raise ValueError( + "a layout section's widgets must be chart ids, {'title': ...}, {'text': ...} or " + f"{{'switcher': [charts]}}, got {section!r}" + ) + continue charts = section["visualizations"] - if not isinstance(charts, list) or not all( - (isinstance(c, str) and c) - or (isinstance(c, dict) and c.keys() == {"title"} and isinstance(c["title"], str) and c["title"]) - for c in charts - ): + if not isinstance(charts, list) or not all(_is_chart_entry(c) for c in charts): raise ValueError( f"a layout section's visualizations must be chart ids or {{'title': ...}}, got {section!r}" ) @@ -1470,8 +1586,94 @@ def _validate_bar_filter(label: str, body: object) -> None: _validate_tab_attribute_filter(label, body) +def _layout_entries(expected_output: dict) -> list[dict]: + """The entries of ``layout``, each naming one layout: a tab's own, or a container on it by path. + + Raises: + ValueError: not a non-empty list of objects, an unknown key, a malformed tab, path or + direction, or one layout named twice. A layout is its tab and its path, the tab's own + layout having none, so one tab can hold several entries. An entry without ``tab`` is + for a dashboard with one tab, so it cannot sit beside an entry that names a tab: both + could name that one tab. + """ + entries = expected_output["layout"] + if not isinstance(entries, list) or not entries or not all(isinstance(e, dict) for e in entries): + raise ValueError(f"layout must be a non-empty list of layout objects, got {entries!r}") + seen: set[tuple[str | None, tuple[tuple[int, int], ...]]] = set() + for entry in entries: + unknown = sorted(entry.keys() - {"tab", "container_path", "layout_direction", "sections"}) + if unknown: + raise ValueError(f"a layout entry cannot state {unknown}, got {entry!r}") + tab = entry.get("tab") + if tab is not None and not (isinstance(tab, str) and tab): + raise ValueError(f"a layout entry's tab must be a tab id, got {entry!r}") + path = _container_path(entry) + if "layout_direction" in entry and (not path or entry["layout_direction"] not in ("row", "column")): + raise ValueError(f"a layout entry's layout_direction is 'row' or 'column' on a container, got {entry!r}") + if (tab, path) in seen: + target = f"the container at {entry['container_path']}" if path else "the layout" + owner = "the dashboard" if tab is None else f"tab {tab!r}" + raise ValueError(f"layout names {target} of {owner} more than once") + seen.add((tab, path)) + tabs = {tab for tab, _ in seen} + if None in tabs and len(tabs) > 1: + raise ValueError(f"a layout entry without tab cannot sit beside one that names a tab, got {entries!r}") + return entries + + +def _container_path(entry: dict) -> tuple[tuple[int, int], ...]: + """A layout entry's ``container_path`` as ``(section, widget_index)`` hops, ``()`` for the tab's own layout. + + Raises: + ValueError: present, but not a non-empty list of hops that each carry exactly a + non-negative ``section`` and ``widget_index``. ``null`` and ``[]`` are refused rather + than read as the tab's own layout: leaving the key out says that. + """ + if "container_path" not in entry: + return () + path = entry["container_path"] + # bool is an int, so `True` would otherwise read as index 1. + if ( + not isinstance(path, list) + or not path + or not all( + isinstance(hop, dict) + and hop.keys() == {"section", "widget_index"} + and all(isinstance(i, int) and not isinstance(i, bool) and i >= 0 for i in hop.values()) + for hop in path + ) + ): + raise ValueError( + f"a layout entry's container_path must be a non-empty list of {{'section', 'widget_index'}} hops, " + f"got {entry!r}" + ) + return tuple((hop["section"], hop["widget_index"]) for hop in path) + + +def _is_chart_entry(entry: object) -> bool: + """A chart as a layout section names it: an id, or ``{"title": ...}`` for one the agent authored.""" + return (isinstance(entry, str) and bool(entry)) or ( + isinstance(entry, dict) + and entry.keys() == {"title"} + and isinstance(entry["title"], str) + and bool(entry["title"]) + ) + + +def _is_widget_entry(entry: object) -> bool: + """A child as a layout section's ``widgets`` names it: a chart, ``{"text": ...}`` or ``{"switcher": [charts]}``.""" + if _is_chart_entry(entry): + return True + if not isinstance(entry, dict) or len(entry) != 1: + return False + if "text" in entry: + return isinstance(entry["text"], str) + charts = entry.get("switcher") + return isinstance(charts, list) and bool(charts) and all(_is_chart_entry(c) for c in charts) + + def _tab_entries(expected_output: dict, key: str, allowed: set[str]) -> list[dict]: - """The entries of a per-tab expectation, checked for the shape ``layout`` and ``filter_bar`` share. + """The entries of a per-tab expectation such as ``filter_bar``, checked for the shape they share. Raises: ValueError: not a non-empty list of objects, a key outside ``allowed``, or a tab named diff --git a/packages/gooddata-eval/tests/test_agentic_dashboard_skill.py b/packages/gooddata-eval/tests/test_agentic_dashboard_skill.py index e7d577df2..9b6075b9f 100644 --- a/packages/gooddata-eval/tests/test_agentic_dashboard_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_dashboard_skill.py @@ -10,6 +10,8 @@ DashboardSkillAssertionError, _date_range_text, _extract_tool_result, + _holds, + _is_container, _skill_activated, _validate_expectation, _widgets_of, @@ -1089,6 +1091,51 @@ def test_widgets_are_read_from_tabs_when_present(self): drafted = _dashboard_part([_widget("A", "id-a")], date_filter=_ALL_TIME_FILTER)["dashboard"] assert [w["visualization"] for w in _widgets_of(drafted)] == ["id-a"] + def test_widgets_inside_containers_are_read_and_a_switcher_stays_whole(self): + switcher = {"visualizations": [{"visualization": "id-c"}, {"visualization": "id-d"}]} + inner = {"container": "inner", "sections": [{"widgets": [_widget("B", "id-b"), switcher]}]} + outer = {"container": "outer", "sections": [{"widgets": [_widget("A", "id-a"), inner]}]} + base = _edit_base_part([outer, _widget("E", "id-e")], visualizations=[])["dashboard"] + assert _widgets_of(base) == [_widget("A", "id-a"), _widget("B", "id-b"), switcher, _widget("E", "id-e")] + + def test_an_empty_container_or_section_holds_no_widgets(self): + """The AAC converter leaves out `sections` of an empty container and `widgets` of an empty section.""" + group = {"container": "g", "sections": [{"title": "Empty"}, {"widgets": [_widget("A", "id-a")]}]} + emptied = {"container": "emptied", "columns": 12, "rows": 10} + base = _edit_base_part([emptied, group], visualizations=[])["dashboard"] + assert _widgets_of(base) == [_widget("A", "id-a")] + + @pytest.mark.parametrize( + ("widget", "container"), + [ + ({"container": "g", "sections": []}, True), + ({"sections": []}, True), + # An empty container: the AAC converter writes `sections` only when there are some. + ({"container": "g", "columns": 12, "rows": 10}, True), + (_widget("A", "id-a"), False), + ({"content": "Europe only"}, False), + ({"id": "s", "visualizations": [{"visualization": "id-a"}]}, False), + ], + ) + def test_a_widget_is_a_container_by_its_container_or_sections_key(self, widget, container): + assert _is_container(widget) is container + + # The converter reads a widget's kind as container, then switcher, then note, then chart. + @pytest.mark.parametrize( + ("widget", "expected", "holds"), + [ + ({"content": "Europe only"}, {"text": "Europe only"}, True), + ({"visualizations": [{"visualization": "id-a"}], "content": "Europe only"}, {"text": "Europe only"}, False), + ({"container": "g", "content": "Europe only"}, {"text": "Europe only"}, False), + ({"visualizations": [{"visualization": "id-a"}]}, {"switcher": ["id-a"]}, True), + ({"container": "g", "visualizations": [{"visualization": "id-a"}]}, {"switcher": ["id-a"]}, False), + (_widget("A", "id-a"), "id-a", True), + ({**_widget("A", "id-a"), "content": "Europe only"}, "id-a", False), + ], + ) + def test_a_child_matches_only_as_the_kind_it_renders_as(self, widget, expected, holds): + assert _holds(widget, set(), expected) is holds + class TestSimulatedReply: def test_existing_charts_are_listed_verbatim(self): @@ -1883,6 +1930,104 @@ def _score_root_edit(expected: dict, operations: list[dict], new_visualizations: _ROOT_EDIT = {**_DE01_EXPECTED, "visualizations": [{"id": _NET_SALES, "title": "Net Sales"}]} +# A container on the overview tab: a note, a drilled chart and a switcher side by side, and beside +# it a second container that nests a third one. Unit-fixture ids, not the live workspace's. +_PRODUCT_BREAKDOWN = "product_breakdown" +_ORDER_VOLUME = "order_volume_by_category" +_NET_SALES_BY_CATEGORY = "net_sales_by_product_category" +_EUROPE_NOTE = "All figures cover Europe only." +_GROUP_PATH = [{"section": 0, "widget_index": 0}] +_INNER_GROUP_PATH = [{"section": 0, "widget_index": 1}, {"section": 0, "widget_index": 1}] + + +def _group(group_id: str, *widgets: dict, direction: str | None = None) -> dict: + group = {"container": group_id, "columns": 12, "rows": 22, "sections": [{"widgets": list(widgets)}]} + return group if direction is None else {**group, "layout_direction": direction} + + +def _switcher(*charts: str) -> dict: + return {"visualizations": [{"visualization": c, "title": c} for c in charts], "columns": 4, "rows": 22} + + +def _grouped_base_part(*, tabbed: bool) -> dict: + product_group = _group( + "product_group", + {"content": _EUROPE_NOTE, "columns": 4, "rows": 22}, + {**_widget("Product breakdown", _PRODUCT_BREAKDOWN, columns=4), "interactions": _DRILL}, + _switcher(_ORDER_VOLUME, _NET_SALES_BY_CATEGORY), + direction="row", + ) + inner_group = _group( + "inner_group", _widget("Active Customers", _ACTIVE_CUSTOMERS), _widget("Return Customers", _RETURN_CUSTOMERS) + ) + kpi_group = _group("kpi_group", _widget("Total Customers", _TOTAL_CUSTOMERS), inner_group) + sections = [{"widgets": [product_group, kpi_group]}] + charts = [_PRODUCT_BREAKDOWN, _ORDER_VOLUME, _NET_SALES_BY_CATEGORY, _TOTAL_CUSTOMERS, _ACTIVE_CUSTOMERS] + part = _edit_base_part(visualizations=[*charts, _RETURN_CUSTOMERS], filters={}) + if tabbed: + part["dashboard"] = { + **part["dashboard"], + "version": "3", + "tabs": [{"id": "overview", "title": "Overview", "filters": {}, "sections": sections}], + } + del part["dashboard"]["sections"], part["dashboard"]["filters"] + else: + part["dashboard"]["sections"] = sections + return part + + +def _group_ops(tabbed: bool, *, swap: bool = True, column: bool = True) -> list[dict]: + """Stack the product group: the chart moves ahead of the note, and the row becomes a column.""" + group = f"{'/tabs/0' if tabbed else ''}/sections/0/widgets/0" + chart = f"{group}/sections/0/widgets/1" + operations = [{"op": "test", "path": f"{chart}/visualization", "value": _PRODUCT_BREAKDOWN}] + if swap: + operations.append({"op": "move", "from": chart, "path": f"{group}/sections/0/widgets/0"}) + if column: + operations.append({"op": "replace", "path": f"{group}/layout_direction", "value": "column"}) + return operations + + +_STACKED = { + "container_path": _GROUP_PATH, + "layout_direction": "column", + "sections": [ + { + "widgets": [ + _PRODUCT_BREAKDOWN, + {"text": _EUROPE_NOTE}, + {"switcher": [_ORDER_VOLUME, _NET_SALES_BY_CATEGORY]}, + ] + } + ], +} +# The inner container carries no layout_direction, which the dashboard lays out as a row. +_INNER_ACTIVE_FIRST = { + "container_path": _INNER_GROUP_PATH, + "layout_direction": "row", + "sections": [{"visualizations": [_ACTIVE_CUSTOMERS, _RETURN_CUSTOMERS]}], +} +_GROUP_EDIT = { + **_DE01_EXPECTED, + "visualizations": [{"id": _PRODUCT_BREAKDOWN, "title": "Product breakdown"}], + "preserved_widgets": [{"visualization": _PRODUCT_BREAKDOWN, "interactions": _DRILL}], +} + + +def _score_group_edit( + layout: list[dict], operations: list[dict], *, tabbed: bool = True, new_visualizations: list[str] | None = None +): + if tabbed: + layout = [{"tab": "overview", **entry} for entry in layout] + patch_part = _patch_part(operations, new_visualizations=new_visualizations) + return evaluate_dashboard_response( + _patch_result(len(new_visualizations or [])), + _grouped_base_part(tabbed=tabbed), + {**_GROUP_EDIT, "layout": layout}, + True, + patch_part=patch_part, + ) + class TestLayout: """A move or a removal is only visible in the whole arrangement: every chart staying "on the @@ -1982,6 +2127,43 @@ def test_an_entry_without_a_tab_on_a_tabbed_dashboard_fails(self): # On a one-tab dashboard both entries could name that tab. [{"sections": []}, {"tab": "tab_customers", "sections": []}], [{"tabs": "tab_customers", "sections": []}], + # A container path is a non-empty list of two non-negative indices per hop. + [{"container_path": None, "sections": []}], + [{"container_path": [], "sections": []}], + [{"container_path": "0/0", "sections": []}], + [{"container_path": [{"section": 0}], "sections": []}], + [{"container_path": [{"section": 0, "widget_index": True}], "sections": []}], + [{"container_path": [{"section": -1, "widget_index": 0}], "sections": []}], + [{"container_path": [{"section": "0", "widget_index": 0}], "sections": []}], + [{"container_path": [{"section": 0, "widget_index": 0, "tab": "x"}], "sections": []}], + # A direction belongs to a container, and is row or column. + [{"layout_direction": "column", "sections": []}], + [{"container_path": _GROUP_PATH, "layout_direction": "diagonal", "sections": []}], + [{"container_path": _GROUP_PATH, "layout_direction": None, "sections": []}], + # A section lists its charts or its widgets, never both. + [{"sections": [{"visualizations": [], "widgets": []}]}], + [{"sections": [{"widgets": "chart"}]}], + *( + [{"sections": [{"widgets": [entry]}]}] + for entry in ( + "", + 3, + {"text": 3}, + {"switcher": []}, + {"switcher": "a"}, + {"switcher": [{"id": "a"}]}, + {"switcher": [{"switcher": ["a"]}]}, + {"chart": "a"}, + {"title": ""}, + {"title": "A", "text": "B"}, + ) + ), + # One target per (tab, path); an unnamed tab still cannot sit beside a named one. + [ + {"tab": "tab_customers", "container_path": _GROUP_PATH, "sections": []}, + {"tab": "tab_customers", "container_path": _GROUP_PATH, "sections": []}, + ], + [{"container_path": _GROUP_PATH, "sections": []}, {"tab": "tab_customers", "sections": []}], ], ) def test_a_malformed_expectation_is_rejected_up_front(self, layout): @@ -1990,6 +2172,221 @@ def test_a_malformed_expectation_is_rejected_up_front(self, layout): _run_with(client, {**_TABBED_EDIT, "layout": layout}) client.send_message.assert_not_called() + @pytest.mark.parametrize( + "layout", + [ + # Distinct containers on one tab, beside the tab's own layout. + [ + {"tab": "tab_customers", "sections": []}, + {"tab": "tab_customers", "container_path": _GROUP_PATH, "layout_direction": "row", "sections": []}, + {"tab": "tab_customers", "container_path": [*_GROUP_PATH, _GROUP_PATH[0]], "sections": []}, + ], + # Several unnamed entries all name the one implicit tab. + [{"sections": []}, {"container_path": _GROUP_PATH, "sections": []}], + [ + { + "container_path": _GROUP_PATH, + "sections": [ + {"title": None, "widgets": ["a", {"title": "B"}, {"text": "C"}, {"switcher": ["d"]}]}, + {"widgets": []}, + ], + } + ], + ], + ) + def test_a_container_expectation_is_accepted(self, layout): + _validate_expectation({**_TABBED_EDIT, "layout": layout}) + + def test_widgets_also_describe_a_tab_layout(self): + layout = [{"tab": "tab_customers", "sections": [{"widgets": [_TOTAL_CUSTOMERS, _NET_SALES]}]}] + swap = [{"op": "move", "from": "/tabs/0/sections/0/widgets/1", "path": "/tabs/0/sections/0/widgets/0"}] + assert _score_tabbed_edit({**_TABBED_EDIT, "layout": layout}, swap).layout_correct + assert not _score_tabbed_edit({**_TABBED_EDIT, "layout": layout}, _RESIZE_AMOUNT).layout_correct + + +class TestContainerLayout: + """A container is checked by its path: its direction and its exact children, charts, notes and + switchers alike. Charts inside it count as on the dashboard for every chart check.""" + + @pytest.mark.parametrize("tabbed", [True, False]) + def test_the_stacked_container_matches(self, tabbed): + ev = _score_group_edit([_STACKED, _INNER_ACTIVE_FIRST], _group_ops(tabbed), tabbed=tabbed) + assert ev.strict_checks["dashboard_layout_correct"] is True + assert ev.strict_checks["dashboard_widgets_preserved"] is True + assert ev.strict_pass, ev.failures + + @pytest.mark.parametrize( + ("operations", "failure"), + [ + pytest.param( + _group_ops(True, swap=False), + "tab 'overview' container at section 0, widget_index 0 section 0 holds " + "[{'text': 'All figures cover Europe only.'}, 'product_breakdown', ", + id="note-and-chart-not-swapped", + ), + pytest.param( + _group_ops(True, column=False), + "tab 'overview' container at section 0, widget_index 0 is laid out as a row, expected a column", + id="left-as-a-row", + ), + pytest.param( + [ + *_group_ops(True), + { + "op": "move", + "from": "/tabs/0/sections/0/widgets/0/sections/0/widgets/2/visualizations/1", + "path": "/tabs/0/sections/0/widgets/0/sections/0/widgets/2/visualizations/0", + }, + ], + "{'switcher': ['net_sales_by_product_category', 'order_volume_by_category']}]", + id="switcher-members-reversed", + ), + pytest.param( + [ + *_group_ops(True), + { + "op": "add", + "path": "/tabs/0/sections/0/widgets/0/sections/0/widgets/-", + "value": _widget("Product breakdown", _PRODUCT_BREAKDOWN), + }, + ], + "'order_volume_by_category', 'net_sales_by_product_category']}, 'product_breakdown']", + id="chart-placed-twice", + ), + pytest.param( + [*_group_ops(True), {"op": "remove", "path": "/tabs/0/sections/0/widgets/0/sections/0/widgets/1"}], + "holds ['product_breakdown', {'switcher':", + id="note-removed", + ), + pytest.param( + [ + *_group_ops(True), + { + "op": "replace", + "path": "/tabs/0/sections/0/widgets/0/sections/0/widgets/2", + "value": _widget("Order volume", _ORDER_VOLUME), + }, + ], + "{'text': 'All figures cover Europe only.'}, 'order_volume_by_category']", + id="switcher-replaced-by-a-chart", + ), + pytest.param( + [ + *_group_ops(True), + { + "op": "replace", + "path": "/tabs/0/sections/0/widgets/0/sections/0/widgets/1/content", + "value": "All figures cover Europe.", + }, + ], + "holds ['product_breakdown', {'text': 'All figures cover Europe.'}, ", + id="note-text-changed", + ), + pytest.param( + [ + *_group_ops(True), + { + "op": "replace", + "path": "/tabs/0/sections/0/widgets/0/sections/0/widgets/0/visualization", + "value": _TOTAL_CUSTOMERS, + }, + ], + f"holds [{_TOTAL_CUSTOMERS!r}, {{'text': ", + id="another-chart-in-place", + ), + pytest.param([], "the patch carries no operations", id="empty-patch"), + ], + ) + def test_a_wrong_arrangement_fails(self, operations, failure): + ev = _score_group_edit([_STACKED], operations) + assert not ev.layout_correct + assert any(failure in f for f in ev.failures), ev.failures + + def test_another_container_on_the_same_tab_is_checked_too(self): + reversed_inner = { + **_INNER_ACTIVE_FIRST, + "sections": [{"visualizations": [_RETURN_CUSTOMERS, _ACTIVE_CUSTOMERS]}], + } + ev = _score_group_edit([_STACKED, reversed_inner], _group_ops(True)) + assert not ev.layout_correct + assert ev.failures == [ + "tab 'overview' container at section 0, widget_index 1 > section 0, widget_index 1 section 0 holds " + f"[{_ACTIVE_CUSTOMERS!r}, {_RETURN_CUSTOMERS!r}], expected [{_RETURN_CUSTOMERS!r}, {_ACTIVE_CUSTOMERS!r}]" + ] + + @pytest.mark.parametrize( + ("path", "failure"), + [ + ([{"section": 0, "widget_index": 5}], "tab 'overview' has no widget at section 0, widget_index 5"), + ([{"section": 1, "widget_index": 0}], "tab 'overview' has no widget at section 1, widget_index 0"), + ( + [*_GROUP_PATH, {"section": 0, "widget_index": 0}], + "tab 'overview' container at section 0, widget_index 0 holds 'product_breakdown' at " + "section 0, widget_index 0, not a container", + ), + ( + [*_GROUP_PATH, {"section": 0, "widget_index": 2}], + "holds {'switcher': ['order_volume_by_category', 'net_sales_by_product_category']} at " + "section 0, widget_index 2, not a container", + ), + ], + ) + def test_a_path_that_reaches_no_container_fails_the_case(self, path, failure): + ev = _score_group_edit([{**_STACKED, "container_path": path}], _group_ops(True)) + assert not ev.layout_correct + assert any(failure in f for f in ev.failures), ev.failures + + def test_an_authored_chart_in_a_container_is_matched_by_title(self): + authored = "b5c2f8d4-0000-4000-8000-000000000001" + operations = [ + *_group_ops(True), + { + "op": "add", + "path": "/tabs/0/sections/0/widgets/0/sections/0/widgets/-", + "value": _widget("Returns by Category", authored), + }, + ] + layout = [ + { + **_STACKED, + "sections": [{"widgets": [*_STACKED["sections"][0]["widgets"], {"title": "returns by category"}]}], + } + ] + assert _score_group_edit(layout, operations, new_visualizations=[authored]).layout_correct + assert not _score_group_edit(layout, operations).layout_correct + + @pytest.mark.parametrize( + ("last", "passes"), + [(_switcher(_ORDER_VOLUME), True), (_widget("Order volume", _ORDER_VOLUME), False)], + ids=["one-chart-switcher", "plain-chart"], + ) + def test_a_one_chart_switcher_is_not_a_chart(self, last, passes): + replace = {"op": "replace", "path": "/tabs/0/sections/0/widgets/0/sections/0/widgets/2", "value": last} + layout = [ + { + **_STACKED, + "sections": [{"widgets": [_PRODUCT_BREAKDOWN, {"text": _EUROPE_NOTE}, {"switcher": [_ORDER_VOLUME]}]}], + } + ] + assert _score_group_edit(layout, [*_group_ops(True), replace]).layout_correct is passes + + def test_tabs_and_chart_checks_see_charts_inside_containers(self): + expected = { + **_GROUP_EDIT, + "visualizations": [ + {"id": _PRODUCT_BREAKDOWN, "title": "Product breakdown"}, + {"id": _ACTIVE_CUSTOMERS, "title": "Active Customers"}, + ], + "tabs": [{"title": "Overview", "visualizations": [_PRODUCT_BREAKDOWN, _ACTIVE_CUSTOMERS]}], + } + patch_part = _patch_part(_group_ops(True)) + ev = evaluate_dashboard_response( + _patch_result(), _grouped_base_part(tabbed=True), expected, True, patch_part=patch_part + ) + assert ev.strict_checks["charts_matched"] is True + assert ev.strict_checks["dashboard_tabs_correct"] is True + assert ev.strict_pass, ev.failures + _CUSTOMER_CONTAINS = {"type": "text_filter", "using": "label/customer_name", "condition": "contains", "value": "Aaron"} # A list filter turned into a text filter as the agent emits it: removed, then added under a minted key.